[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
WalkthroughAdds Lucide web-icon import and SVG manipulation features across libraries and the IDE plugin. Introduces SvgManipulator (DOM-based SVG utilities) and its tests, extends SVG API, adds a colored LucideLogo ImageVector, and updates build/dependencies. The IDE plugin gains a Lucide import flow: LucideImportScreen, LucideViewModel, LucideRepository, LucideUseCase, domain models (Category, LucideIcon, LucideConfig, LucideSettings), DI modules (NetworkModule, LucideModule), UI components (customization panel, icon display, top actions), a NoStopIndicatorSlider, and new resource strings. Possibly related issues
🚥 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
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
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
Show resolved
Hide resolved
| is IconItem<*> -> { | ||
| val lucideIcon = item.icon as LucideIcon | ||
| val iconCacheKey = getIconCacheKey(lucideIcon.name, state.settings) | ||
| var iconLoadState = state.loadedIcons[iconCacheKey] | ||
|
|
||
| if (iconLoadState !is IconLoadState.Success) { | ||
| iconLoadState = state.loadedIcons.entries | ||
| .firstOrNull { | ||
| it.key.startsWith("${lucideIcon.name}-") && | ||
| it.value is IconLoadState.Success | ||
| } | ||
| ?.value | ||
| } |
There was a problem hiding this comment.
Unchecked cast and potential performance concern in fallback logic.
-
Line 192: The unchecked cast
item.icon as LucideIconcould throw at runtime if the grid ever contains non-Lucide icons. Consider using a type-safe approach or ensuringIconItem<LucideIcon>at the grid level. -
Lines 196-203: The fallback logic performs a linear scan through all
loadedIconsentries on every recomposition for icons not yet loaded with current settings. For large icon sets (Lucide has 1500+ icons), this could cause frame drops.
Suggested improvement for the fallback lookup
Consider caching the fallback lookup or restructuring the cache to enable O(1) lookups by icon name:
- if (iconLoadState !is IconLoadState.Success) {
- iconLoadState = state.loadedIcons.entries
- .firstOrNull {
- it.key.startsWith("${lucideIcon.name}-") &&
- it.value is IconLoadState.Success
- }
- ?.value
- }
+ val displayState = iconLoadState.takeIf { it is IconLoadState.Success }
+ ?: state.getLatestSuccessfulState(lucideIcon.name)Then add a helper in LucideState.Success that maintains a separate index by icon name.
...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.
| suspend fun loadIconList(): List<Pair<String, LucideIconMetadata>> = withContext(Dispatchers.IO) { | ||
| val response = httpClient.get("$UNPKG_BASE/tags.json") | ||
| val tagsJson = json.parseToJsonElement(response.bodyAsText()) as JsonObject | ||
|
|
||
| tagsJson.entries.map { (iconName, tagsArray) -> | ||
| val tags = tagsArray.jsonArray.map { it.jsonPrimitive.content } | ||
| iconName to LucideIconMetadata( | ||
| tags = tags, | ||
| categories = emptyList(), // Categories will be inferred from tags | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt | head -60Repository: ComposeGears/Valkyrie
Length of output: 2870
🌐 Web query:
lucide-static tags.json structure format
💡 Result:
Structure: tags.json is a plain JSON object that maps each icon name (kebab-case filename/slug) to an array of tag strings.
- top-level type: object (dictionary). [1]
- keys: icon names (e.g. "mail", "arrow-left", "pin-off").
- values: arrays of tag strings (lowercase keywords) describing that icon (e.g. ["mail","message","communication"]). [2][3]
Example:
{
"mail": ["email","message","letter"],
"arrow-left": ["navigation","back","previous"],
"pin-off": ["pin","unpin","map"]
}
Sources:
- lucide-static package listing / tags.json file (UNPKG). [1]
- Lucide tags & categories documentation / metadata tools (migrate scripts & usage). [2][3]
References:
[1] UNPKG — lucide-static (tags.json present in package). (app.unpkg.com)
[2] Lucide docs / Lucide Static overview. (lucide.dev)
[3] Tags and Categories (documentation / DeepWiki summary of tags.json usage). (deepwiki.com)
Citations:
- 1: https://app.unpkg.com/lucide-static%400.561.0/files/tags.json
- 2: https://lucide.dev/guide/packages/lucide-static?utm_source=openai
- 3: https://deepwiki.com/lucide-icons/lucide/2.2-tags-and-categories?utm_source=openai
Add safe casting and error handling for JSON response.
Line 43 uses an unsafe cast that could throw a ClassCastException if the response structure is unexpected. While lucide-static's tags.json is stable and reliably returns a JSON object, defensive error handling is a best practice:
Suggested fix
- val tagsJson = json.parseToJsonElement(response.bodyAsText()) as JsonObject
+ val tagsJson = json.parseToJsonElement(response.bodyAsText()) as? JsonObject
+ ?: throw IllegalStateException("Invalid tags.json format: expected JSON object")Note: The code also lacks HTTP-level error handling (e.g., status codes, timeouts); consider adding that as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| suspend fun loadIconList(): List<Pair<String, LucideIconMetadata>> = withContext(Dispatchers.IO) { | |
| val response = httpClient.get("$UNPKG_BASE/tags.json") | |
| val tagsJson = json.parseToJsonElement(response.bodyAsText()) as JsonObject | |
| tagsJson.entries.map { (iconName, tagsArray) -> | |
| val tags = tagsArray.jsonArray.map { it.jsonPrimitive.content } | |
| iconName to LucideIconMetadata( | |
| tags = tags, | |
| categories = emptyList(), // Categories will be inferred from tags | |
| ) | |
| } | |
| } | |
| suspend fun loadIconList(): List<Pair<String, LucideIconMetadata>> = withContext(Dispatchers.IO) { | |
| val response = httpClient.get("$UNPKG_BASE/tags.json") | |
| val tagsJson = json.parseToJsonElement(response.bodyAsText()) as? JsonObject | |
| ?: throw IllegalStateException("Invalid tags.json format: expected JSON object") | |
| tagsJson.entries.map { (iconName, tagsArray) -> | |
| val tags = tagsArray.jsonArray.map { it.jsonPrimitive.content } | |
| iconName to LucideIconMetadata( | |
| tags = tags, | |
| categories = emptyList(), // Categories will be inferred from tags | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt
around lines 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.
There was a problem hiding this comment.
The current code is fine. The outer runCatching already provides proper error handling and user feedback.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of loadIconList to verify error handling context
rg -n -A5 -B5 'loadIconList\s*\(' tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/
Length of output: 3495
🏁 Script executed:
#!/bin/bash
# Search for usages of loadConfig to verify runCatching wrapper
rg -n -A10 -B5 'loadConfig\s*\(' tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt
Length of output: 1322
@t-regbs, you're absolutely right! I can see the runCatching block in LucideViewModel.loadConfig() (lines 98-107) that wraps the loadConfig() call and provides proper error handling with .onFailure that logs the error and updates the state to show a user-friendly error message. This catches any exceptions from the repository layer, including potential JSON parsing issues.
The error handling strategy is appropriate for this architecture. Thanks for the clarification! 🐰
…r against xxe attacks
t-regbs
force-pushed
the
feature/lucide-web-import
branch
from
568c771 to
093ac0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/di/NetworkModule.kt:
- Around line 21-30: The httpClient binding should be made a singleton like the
json binding by adding keepAlive = true to the instanceOf declaration; update
the val httpClient by instanceOf { ... } for HttpClient(OkHttp) (the
HttpClient/OkHttp initialization block) to include keepAlive = true so a single
shared HttpClient instance is used across web import features.
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt:
- Around line 94-97: The Color.toHexString function uses String.format with the
implicit default locale which can vary; update the call to use a
locale-independent formatter (e.g., pass Locale.US or Locale.ROOT as the first
argument) so the hex formatting is deterministic across locales while keeping
the same format string and bitmask logic in toHexString.
- Around line 42-43: The code unsafely casts the parsed JSON to JsonObject
(tagsJson = json.parseToJsonElement(... ) as JsonObject) which can throw; change
to parse to a JsonElement, then verify it is a JsonObject (e.g., val element =
json.parseToJsonElement(response.bodyAsText()); if (element is JsonObject) { val
tagsJson = element } else { handle invalid format: log error via your logger in
LucideRepository, return/throw a descriptive exception, or fallback to empty
object }); use the jsonObject-safe accessor or type check instead of the unsafe
cast to avoid ClassCastException.
- Around line 54-66: In getRawSvg, the current separate locking allows multiple
coroutines to fetch the same icon; either hold cacheMutex for the whole
check+fetch+put by moving the HTTP call inside cacheMutex.withLock around
rawSvgCache and downloaded storage, or implement a per-icon in-flight map (e.g.,
ongoingRequests: MutableMap<String, Deferred<String>>) so callers await the same
Deferred; reference getRawSvg, cacheMutex, rawSvgCache and httpClient (or create
ongoingRequests and use coroutineScope.async/await) to ensure only one network
fetch per icon.
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt:
- Around line 222-227: You captured `state` before acquiring `mutex` so the
stale `currentState` may miss concurrent updates; inside the `mutex.withLock`
block re-read `state.loadedIcons[cacheKey]` (e.g. assign `val currentState =
state.loadedIcons[cacheKey]` inside the lock) and use that value for the `if
(currentState is IconLoadState.Success || currentState is IconLoadState.Loading)
return@launch` check, removing reliance on the earlier captured value so the
decision reflects the protected, up-to-date state.
- Around line 254-257: The code recomputes the cache key when registering and
removing jobs which can mismatch if settings change; capture the cache key once
into a local val (e.g., cacheKey = buildIconCacheKey(icon.name,
lucideRecord.value.safeAs<LucideState.Success>()?.settings ?: LucideSettings()))
and then use that cacheKey for both iconLoadJobs[cacheKey] = job and inside
job.invokeOnCompletion { iconLoadJobs.remove(cacheKey) } so the same key is used
consistently (references: iconLoadJobs, buildIconCacheKey, lucideRecord,
LucideState.Success, LucideSettings, job).
- Around line 339-344: The updateSuccess function performs a non-atomic
read-modify-write on lucideRecord causing lost updates when called from multiple
coroutines; convert it to a suspend function and protect the critical section
with a Mutex (e.g., a private val lucideMutex = Mutex()), then acquire the mutex
in updateSuccess, re-read lucideRecord, apply the transform only while holding
the lock and update lucideRecord before releasing the lock; updateSuccess and
any call sites are already in suspend contexts so switch the signature to
suspend and serialize mutations to LucideState.Success using the Mutex to
prevent races.
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt:
- Around line 38-39: The local vars strokeWidth and size are initialized from
settings using remember so they won't update when settings changes; update the
initialization to recreate or derive values when settings changes (e.g., use
remember(key = settings.strokeWidth) with mutableFloatStateOf for strokeWidth
and remember(key = settings.size) with mutableIntStateOf for size, or use
derivedStateOf tied to settings.strokeWidth/settings.size) so the local state in
LucideCustomization reflects external updates to settings.
- Around line 72-75: The decimal formatting for the strokeWidth label uses the
platform default locale via String.format("%.1f", strokeWidth) which can produce
commas in some locales; change the call in LucideCustomization.kt (the Text that
shows "Stroke width: ${String.format(...)}") to use an explicit locale (e.g.,
Locale.US) so decimals always render with a dot, i.e., replace
String.format("%.1f", strokeWidth) with a locale-aware variant such as
String.format(Locale.US, "%.1f", strokeWidth) inside the Text composable.
🧹 Nitpick comments (7)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt (1)
47-50: Error state shows identical UI to loading state.When
iconLoadStateisError, users see the same shimmer placeholder as during loading, providing no feedback that something went wrong. Consider showing a distinct error indicator (e.g., an error icon or retry option) to improve UX.components/parser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt (1)
33-59: Replaceprintlnwith proper logging.Using
printlnfor error logging (line 56) is not appropriate for an IDE plugin. It may not be visible to users and doesn't integrate with IntelliJ's logging infrastructure.♻️ Suggested fix
Consider using a logger or simply removing the print statement since the function already gracefully returns the original content:
} catch (e: Exception) { - println("Failed to parse SVG for modification: ${e.message}") + // Return original content on parse failure svgContent }Or if logging is needed, use IntelliJ's logger:
private val LOG = com.intellij.openapi.diagnostic.Logger.getInstance(SvgManipulator::class.java) // ... LOG.warn("Failed to parse SVG for modification", e)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt (1)
107-109: Priority boost creates negative values.The
-2boost for name matches can result in negative priority values (e.g., priority 1 becomes -1). WhileminByOrNullhandles this correctly, it could be confusing for future maintainers. Consider documenting this or using a multiplicative factor instead.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (4)
48-49: Unbounded growth oficonLoadMutexesmap.The
iconLoadMutexesmap grows indefinitely as new cache keys are encountered but entries are never removed. UnlikeiconLoadJobswhich cleans up viainvokeOnCompletion, mutexes persist indefinitely. Over many settings changes, this could accumulate thousands of stale mutex entries.Consider either:
- Using a bounded cache for mutexes similar to
iconVectorCache- Cleaning up mutexes when their corresponding cache entries are evicted
- Using a single global mutex if contention is low enough
126-128: Silent failure: no user feedback when icon download fails.When
downloadIconfails, the error is logged but the UI receives no notification. The user initiates a download action and gets no feedback on failure.Consider emitting an error event (e.g.,
LucideEvent.DownloadFailed) so the UI can display a toast or error message.💡 Suggested approach
sealed interface LucideEvent { data class IconDownloaded(...) : LucideEvent data class DownloadFailed(val iconName: String, val message: String) : LucideEvent } // In downloadIcon: }.onFailure { error -> LOG.error("Failed to download icon '${icon.name}'", error) _events.emit(LucideEvent.DownloadFailed(icon.name, error.message ?: "Unknown error")) }
224-226: Failed icons will retry on every display request.The check skips re-loading for
SuccessorLoadingstates but notError. This means icons that previously failed will retry on each visibility change, potentially causing repeated network requests.If retry is intentional, consider adding a backoff or max retry count. If not, add
Errorto the skip condition:-if (currentState is IconLoadState.Success || currentState is IconLoadState.Loading) { +if (currentState is IconLoadState.Success || currentState is IconLoadState.Loading || currentState is IconLoadState.Error) { return@launch }
370-371: Unbounded growth ofloadedIconsmap vs boundediconVectorCache.The
iconVectorCacheLruCache is bounded at 300 entries, butloadedIconsin state grows without bound. Over time, the state map can accumulate thousands of entries (one per icon + settings combination), while the actual vectors may have been evicted from the LRU cache.Consider periodically pruning
loadedIconsor synchronizing its size with the cache, especially when settings change and old entries become stale.
📜 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/domain/model/Category.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportFlow.kt
- compose/icons/api/icons.api
- gradle/libs.versions.toml
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideConfig.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kt
- 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/common/ui/Slider.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/WebImportSelectorScreen.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/di/LucideModule.kt
- components/parser/jvm/svg/api/svg.api
🧰 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/domain/model/LucideGridItem.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.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:
tools/idea-plugin/build.gradle.kts
🧬 Code graph analysis (1)
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)
🪛 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)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt
[warning] 73-73: String.format("%.1f", strokeWidth) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🔇 Additional comments (20)
compose/icons/api/icons.klib.api (1)
21-22: LGTM!The new
LucideLogoAPI entry correctly follows the established pattern for colored icons in this library. The declaration is properly ordered alphabetically betweenGoogleMaterialLogoandValkyrieLogo, and uses the expected extension property pattern returningImageVector.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt (3)
52-61: Reset logic is well implemented.The reset button correctly updates both local state variables and propagates the default settings via
onSettingsChange. Enabling the button conditionally based onsettings.isModifiedprovides good UX feedback.
77-85: Slider configuration looks correct.The
onValueChange/onValueChangeFinishedpattern properly separates intermediate drag updates from final commits, avoiding excessive reparsing during slider interaction. This aligns with the PR discussion about reducing unnecessary reparsing.Also applies to: 94-102
106-125: Switch implementation is clean.The
absoluteStrokeWidthtoggle correctly propagates changes immediately viaonSettingsChange, which is appropriate since switches don't have continuous drag interaction like sliders.components/parser/jvm/svg/build.gradle.kts (1)
9-10: LGTM!The test dependency addition aligns with the new
SvgManipulatorTest.kttest suite for the SVG manipulation utilities.tools/idea-plugin/build.gradle.kts (2)
30-30: LGTM!Adding the SVG parser module enables the Lucide feature to use
SvgManipulatorfor DOM-based SVG modifications.
70-70: LGTM!The
androidx.collectiondependency provides theLruCacheimplementation used inLucideRepositoryfor caching SVG strings.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideGridItem.kt (1)
10-15: LGTM!Clean extension function that properly sorts categories and flattens the map into a display-ready grid structure.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideIconMetadata.kt (1)
12-15: Thetagsandcategoriesfields do not require default values. TheLucideIconMetadataobject is manually constructed inloadIconList()with explicit values—tags are always populated from the parsedtags.jsonAPI, and categories are intentionally always initialized asemptyList()to be inferred later from tags. Automatic deserialization of this class from API responses does not occur.Likely an incorrect or invalid review comment.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideIcon.kt (1)
3-8: LGTM!Clean and minimal domain model. The data class is well-structured with appropriate immutability.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideTopActions.kt (1)
9-31: LGTM!Clean composition pattern that reuses the shared
WebImportTopActionscomponent while providing Lucide-specific category filtering. The parameter ordering follows Compose conventions correctly.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt (1)
35-41: Consider checking existing state before triggering load.The
DisposableEffectunconditionally triggersonLoadIconwhenevericonCacheKeychanges, even if the icon is already inSuccessstate. This could cause unnecessary network requests if the parent doesn't handle deduplication.If the ViewModel already handles this deduplication (e.g., by checking cache before fetching), this is fine. Otherwise, consider adding a guard:
DisposableEffect(iconCacheKey) { val job = if (iconLoadState !is IconLoadState.Success) { currentOnLoadIcon(icon) } else null onDispose { job?.cancel() } }components/parser/jvm/svg/src/test/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulatorTest.kt (1)
11-250: LGTM!Comprehensive test coverage including root attribute updates, recursive traversal, conditional updates, nested elements, quote variations, parse failure handling, and a real-world Lucide icon scenario. Well-structured test cases with clear naming.
components/parser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt (1)
34-39: Good security hardening for XML parsing.The XXE prevention measures (disabling doctype declarations and external entities) are correctly implemented, protecting against XML External Entity attacks.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt (2)
69-92: Consider error handling forloadConfig.The
loadConfig()function doesn't handle potential exceptions fromrepository.loadIconList(). If the HTTP request fails, the exception will propagate up. Consider whether this is the intended behavior or if a fallback/error state should be returned.
103-120: Substring matching may cause unexpected categorization.The
contains()check (lines 108, 113) matches substrings, which could lead to unexpected results:
- "context" would match "text" → categorized as "Text"
- "unlock" would match "lock" → categorized as "Security"
If this is intentional behavior (and it seems reasonable for icon categorization), this is fine. Otherwise, consider word-boundary matching.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (4)
69-92: Well-structured initialization with debounced search.The init block properly handles state restoration and sets up debounced search filtering. The use of
Dispatchers.Defaultfor the collect block is appropriate for CPU-bound filtering work.
94-109: LGTM: Clean async loading pattern.The
loadConfigfunction follows a good pattern with loading state, error handling, and appropriate logging.
313-337: LGTM: Well-implemented filter logic.The
filterGridItemsfunction is a clean pure function with efficient filtering across categories and search terms.
360-378: Clean sealed interface design with@Stableannotations.The state hierarchy is well-structured. The
iconNameIndexprovides efficient lookup by icon name for thegetLatestSuccessfulStatehelper.
| val httpClient by instanceOf { | ||
| HttpClient(OkHttp) { | ||
| install(HttpTimeout) { | ||
| requestTimeoutMillis = 30.seconds.inWholeMilliseconds | ||
| } | ||
| install(ContentNegotiation) { | ||
| json(inject(json)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add keepAlive = true to httpClient for proper singleton behavior.
The json binding uses keepAlive = true to ensure a single instance, but httpClient doesn't. Creating multiple HttpClient instances wastes resources since OkHttp maintains connection pools internally. This also aligns with the reviewer feedback about sharing a single client across web import features.
Suggested fix
- val httpClient by instanceOf {
+ val httpClient by instanceOf(keepAlive = true) {
HttpClient(OkHttp) {🤖 Prompt for AI Agents
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/di/NetworkModule.kt
around lines 21 - 30, The httpClient binding should be made a singleton like the
json binding by adding keepAlive = true to the instanceOf declaration; update
the val httpClient by instanceOf { ... } for HttpClient(OkHttp) (the
HttpClient/OkHttp initialization block) to include keepAlive = true so a single
shared HttpClient instance is used across web import features.
There was a problem hiding this comment.
The httpClient binding doesn't need keepAlive = true because Leviathan's instanceOf already provides singleton behavior by default.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Leviathan source code or documentation about instanceOf behavior
echo "=== Searching for Leviathan class definition ==="
rg -n "class Leviathan|object Leviathan" --type=kt -A 10
echo -e "\n=== Searching for instanceOf function definition ==="
rg -n "fun.*instanceOf|inline.*instanceOf" --type=kt -A 5
echo -e "\n=== Searching for keepAlive parameter usage ==="
rg -n "keepAlive" --type=kt -C 2
echo -e "\n=== Searching for other instanceOf usages in DI modules ==="
rg -n "by instanceOf" --type=kt -g "*Module.kt" -C 1
Length of output: 320
🏁 Script executed:
#!/bin/bash
# Search for Leviathan source code or documentation about instanceOf behavior
echo "=== Searching for Leviathan class definition ==="
rg -n "class Leviathan|object Leviathan" -g "*.kt" -A 10
echo -e "\n=== Searching for instanceOf function definition ==="
rg -n "fun.*instanceOf|inline.*instanceOf" -g "*.kt" -A 5
echo -e "\n=== Searching for keepAlive parameter usage ==="
rg -n "keepAlive" -g "*.kt" -C 2
echo -e "\n=== Searching for other instanceOf usages in DI modules ==="
rg -n "by instanceOf" -g "*Module.kt" -C 1
Length of output: 4989
@t-regbs, you're absolutely right, and I apologize for the incorrect suggestion. Looking at the codebase patterns, I can see that most instanceOf declarations (like lucideRepository, lucideUseCase, materialSymbolsConfigRepository, etc.) don't use keepAlive = true, which confirms that instanceOf provides singleton behavior by default in Leviathan. The keepAlive = true parameter appears to be used only for specific cases that require explicit keep-alive semantics. Thank you for the clarification!
In Leviathan DI framework, instanceOf provides singleton behavior by default, so keepAlive = true is not needed unless specific keep-alive semantics are required. Most DI bindings in the codebase use instanceOf without the keepAlive parameter.
| val response = httpClient.get("$UNPKG_BASE/tags.json") | ||
| val tagsJson = json.parseToJsonElement(response.bodyAsText()) as JsonObject |
There was a problem hiding this comment.
Unsafe cast may throw on unexpected JSON format.
The cast as JsonObject on line 43 will throw ClassCastException if tags.json has an unexpected format (e.g., array at root level). Consider using a safer approach:
🛡️ Suggested defensive handling
- val tagsJson = json.parseToJsonElement(response.bodyAsText()) as JsonObject
+ val tagsJson = json.parseToJsonElement(response.bodyAsText()) as? JsonObject
+ ?: throw IllegalStateException("Invalid tags.json format: expected JSON object")🤖 Prompt for AI Agents
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt
around lines 42 - 43, The code unsafely casts the parsed JSON to JsonObject
(tagsJson = json.parseToJsonElement(... ) as JsonObject) which can throw; change
to parse to a JsonElement, then verify it is a JsonObject (e.g., val element =
json.parseToJsonElement(response.bodyAsText()); if (element is JsonObject) { val
tagsJson = element } else { handle invalid format: log error via your logger in
LucideRepository, return/throw a descriptive exception, or fallback to empty
object }); use the jsonObject-safe accessor or type check instead of the unsafe
cast to avoid ClassCastException.
There was a problem hiding this comment.
The current code is fine. The outer runCatching already provides proper error handling and user feedback.
There was a problem hiding this comment.
@t-regbs, understood! You're right that runCatching at the call site will handle any exceptions including ClassCastException.
While safe casting with "as?" helps ensure type compatibility and could provide clearer error messages if the format is unexpected, if you're satisfied with the current error handling approach that's perfectly fine. The outer runCatching will indeed catch and handle the exception appropriately.
Thanks for clarifying your error handling strategy! 🐰
...n/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt
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
| private inline fun updateSuccess(crossinline transform: (LucideState.Success) -> LucideState.Success) { | ||
| val current = lucideRecord.value | ||
| if (current is LucideState.Success) { | ||
| lucideRecord.value = transform(current) | ||
| } | ||
| } |
There was a problem hiding this comment.
Race condition: non-atomic read-modify-write in updateSuccess.
This function is called from multiple coroutines concurrently (debounced search, category selection, settings updates, icon parsing). The read at line 340 and write at line 342 are not atomic—concurrent calls can overwrite each other's changes, causing lost updates.
For example, if two icons finish parsing simultaneously, only one may be added to loadedIcons because both read the same state before writing.
🔒️ Proposed fix: use a Mutex to serialize state updates
+ private val stateMutex = Mutex()
+
- private inline fun updateSuccess(crossinline transform: (LucideState.Success) -> LucideState.Success) {
- val current = lucideRecord.value
- if (current is LucideState.Success) {
- lucideRecord.value = transform(current)
+ 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 requires updating all call sites to be in a suspend context, which they already are.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private inline fun updateSuccess(crossinline transform: (LucideState.Success) -> LucideState.Success) { | |
| val current = lucideRecord.value | |
| if (current is LucideState.Success) { | |
| lucideRecord.value = transform(current) | |
| } | |
| } | |
| private val stateMutex = Mutex() | |
| 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) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt
around lines 339 - 344, The updateSuccess function performs a non-atomic
read-modify-write on lucideRecord causing lost updates when called from multiple
coroutines; convert it to a suspend function and protect the critical section
with a Mutex (e.g., a private val lucideMutex = Mutex()), then acquire the mutex
in updateSuccess, re-read lucideRecord, apply the transform only while holding
the lock and update lucideRecord before releasing the lock; updateSuccess and
any call sites are already in suspend contexts so switch the signature to
suspend and serialize mutations to LucideState.Success using the Mutex to
prevent races.
There was a problem hiding this comment.
This is not a race condition in practice. The lucideRecord is backed by Tiamat's MutableSavedState.recordOf, which internally uses a StateFlow - and StateFlow updates are atomic and thread-safe by design.
Adding a Mutex here would unnecessarily serialize all state updates and hurt performance without meaningful benefit.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Tiamat Compose navigation MutableSavedState recordOf implementation
💡 Result:
I couldn't find any public implementation or reference for a "recordOf" function on Tiamat's Compose navigation + MutableSavedState. I searched Tiamat docs/articles and Jetpack Compose SavedState docs but didn't locate a recordOf symbol or example — only general saved-state guidance and Tiamat overview. See Tiamat intro and Compose saved-state docs for reference. [1][2]
If you meant something specific, please either:
- share the code or repo link where recordOf appears, or
- clarify the exact signature/behavior you expect (e.g., convert MutableSavedState to an immutable record/data class, create a Saver for MutableState, or a navigation-state snapshot API),
and I’ll find or draft an implementation.
Sources:
- Tiamat overview/article. [1] (medium.com)
- Jetpack Compose saved-state / SavedStateHandle docs. [2] (developer.android.com)
Citations:
- 1: https://medium.com/mobilepeople/tiamat-simple-compose-multiplatform-navigation-3956dc00a607?utm_source=openai
- 2: https://developer.android.com/develop/ui/compose/state-saving?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Search for Tiamat's recordOf implementation in the codebase
rg -n "fun recordOf" --type kotlin -A 10
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for recordOf usage to understand its behavior
rg -n "recordOf" --type kotlin -B 2 -A 5
# Also search for MutableSavedState to understand the API
rg -n "MutableSavedState" --type kotlin -B 2 -A 5 | head -50
Length of output: 22288
🏁 Script executed:
#!/bin/bash
# Check MaterialSymbolsViewModel for similar update patterns
cat tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/MaterialSymbolsViewModel.kt | grep -A 10 "fun update\|private.*fun.*update\|materialRecord.value ="
Length of output: 2250
@t-regbs You're absolutely correct that StateFlow's individual write operations are atomic. However, the issue I flagged is about the read-modify-write pattern, not the atomicity of individual writes.
The problem is in this sequence:
private inline fun updateSuccess(crossinline transform: (LucideState.Success) -> LucideState.Success) {
val current = lucideRecord.value // ← Read
if (current is LucideState.Success) {
lucideRecord.value = transform(current) // ← Write
}
}Even with atomic writes, two concurrent executions can interleave:
- Coroutine A reads
lucideRecord.value(let's say it hasloadedIcons = {icon1: Success}) - Coroutine B reads
lucideRecord.value(same state:loadedIcons = {icon1: Success}) - Coroutine A writes
loadedIcons = {icon1: Success, icon2: Success} - Coroutine B writes
loadedIcons = {icon1: Success, icon3: Success}← icon2 is lost
This is the classic lost update problem in concurrent programming.
That said, I see MaterialSymbolsViewModel uses the identical pattern, and you clearly know this codebase well. If you've tested this thoroughly and haven't seen issues in practice, the race window may be small enough that it doesn't manifest under normal usage patterns (debounced search, sequential user actions, etc.).
I'll defer to your judgment here—if the current approach works reliably, there's no need to add synchronization overhead. Just wanted to clarify the technical distinction between atomic writes and atomic read-modify-write operations. 🐰
| var strokeWidth by remember { mutableFloatStateOf(settings.strokeWidth) } | ||
| var size by remember { mutableIntStateOf(settings.size) } |
There was a problem hiding this comment.
Local state won't sync if settings prop changes externally.
The local strokeWidth and size states are initialized from settings using remember, which only captures the initial value. If settings is updated externally (e.g., reset triggered from another component or parent recomposition with new values), the UI will display stale local state.
Consider using derivedStateOf or updating local state when the key changes:
Proposed fix using remember with key
- 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) }Note: Using settings.strokeWidth / settings.size as keys will reset local state when the external settings change, which is the expected behavior for a controlled component pattern.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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) } |
🤖 Prompt for AI Agents
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt
around lines 38 - 39, The local vars strokeWidth and size are initialized from
settings using remember so they won't update when settings changes; update the
initialization to recreate or derive values when settings changes (e.g., use
remember(key = settings.strokeWidth) with mutableFloatStateOf for strokeWidth
and remember(key = settings.size) with mutableIntStateOf for size, or use
derivedStateOf tied to settings.strokeWidth/settings.size) so the local state in
LucideCustomization reflects external updates to settings.
| Text( | ||
| text = "Stroke width: ${String.format("%.1f", strokeWidth)}", | ||
| fontWeight = FontWeight.Medium, | ||
| ) |
There was a problem hiding this comment.
Use explicit locale for consistent decimal formatting.
String.format("%.1f", strokeWidth) uses the implicit default locale, which may produce a comma as the decimal separator in some locales (e.g., "2,5" instead of "2.5"), potentially causing inconsistent UI display.
Proposed fix
+import java.util.Locale
+
Text(
- text = "Stroke width: ${String.format("%.1f", strokeWidth)}",
+ text = "Stroke width: ${String.format(Locale.US, "%.1f", strokeWidth)}",
fontWeight = FontWeight.Medium,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Text( | |
| text = "Stroke width: ${String.format("%.1f", strokeWidth)}", | |
| fontWeight = FontWeight.Medium, | |
| ) | |
| Text( | |
| text = "Stroke width: ${String.format(Locale.US, "%.1f", strokeWidth)}", | |
| fontWeight = FontWeight.Medium, | |
| ) |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 73-73: String.format("%.1f", strokeWidth) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🤖 Prompt for AI Agents
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt
around lines 72 - 75, The decimal formatting for the strokeWidth label uses the
platform default locale via String.format("%.1f", strokeWidth) which can produce
commas in some locales; change the call in LucideCustomization.kt (the Text that
shows "Stroke width: ${String.format(...)}") to use an explicit locale (e.g.,
Locale.US) so decimals always render with a dot, i.e., replace
String.format("%.1f", strokeWidth) with a locale-aware variant such as
String.format(Locale.US, "%.1f", strokeWidth) inside the Text composable.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 55-64: In getRawSvg, avoid holding cacheMutex across network I/O:
first acquire cacheMutex and check rawSvgCache[iconName]; if present return it
and release the lock; if absent release the lock, perform the
httpClient.get(...) download from "$UNPKG_BASE/icons/$iconName.svg", then
re-acquire cacheMutex and put the downloaded value into rawSvgCache only if the
entry is still missing (double-check) before returning; reference cacheMutex,
rawSvgCache, getRawSvg, httpClient and UNPKG_BASE to locate the changes.
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt:
- Around line 337-342: updateSuccess performs a non-atomic read-modify-write on
lucideRecord.value which can lose concurrent updates; change the implementation
to perform an atomic update (e.g., use MutableStateFlow.update { current -> if
(current is LucideState.Success) transform(current) else current } or implement
a compare-and-set loop) so multiple coroutines merging parsed icons won't
overwrite each other; make updateSuccess suspend if you choose an atomic
suspendable approach or convert lucideRecord to a MutableStateFlow and call its
update { } helper and update all call sites accordingly.
🧹 Nitpick comments (5)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt (2)
42-53: Consider safer JSON parsing with explicit error handling.The cast
as JsonObjectwill throwClassCastExceptionif the response isn't a JSON object. Consider using a safer approach or catching exceptions to provide a more meaningful error message.💡 Suggested improvement
suspend fun loadIconList(): List<Pair<String, LucideIconMetadata>> = withContext(Dispatchers.IO) { val response = httpClient.get("$UNPKG_BASE/tags.json") - val tagsJson = json.parseToJsonElement(response.bodyAsText()) as JsonObject + val tagsJson = json.parseToJsonElement(response.bodyAsText()) as? JsonObject + ?: error("Expected JSON object from tags.json") tagsJson.entries.map { (iconName, tagsArray) ->
66-74: Float comparison with!=may be fragile.Comparing floats with
!=can be unreliable due to floating-point precision. IfstrokeWidthcould result from calculations or slider interpolation, consider using an epsilon comparison or comparing the underlying representation.💡 Suggested improvement
-if (settings.strokeWidth != DEFAULT_STROKE_WIDTH.toFloat()) { +if (kotlin.math.abs(settings.strokeWidth - DEFAULT_STROKE_WIDTH.toFloat()) > 0.001f) {Alternatively, if slider values are guaranteed to be exact, this may be acceptable as-is.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (3)
47-48: Unbounded growth oficonLoadMutexesmap.Mutexes are added via
computeIfAbsentinloadIconForDisplaybut never removed. With many unique cache keys (icon name + settings combinations), this map can grow indefinitely during a session.Consider periodically cleaning up unused mutexes or using a bounded cache for mutexes similar to
iconVectorCache.💡 Suggested approach
One option is to clean up the mutex when the job completes and no other operations are pending:
private val iconLoadMutexes = ConcurrentHashMap<String, Mutex>() private val mutexRefCounts = ConcurrentHashMap<String, Int>() // track usage // Or simpler: just accept that mutexes are cheap and let them grow, // but document the expected upper bound based on LruCache sizeAlternatively, since the
iconVectorCacheis bounded to 300 entries, you could tie mutex lifecycle to cache evictions.
154-168: Side effects inside state transform function.The
updateSuccesstransform at lines 157-168 mutates external variables (iconsToReload,oldCacheKeys) while also returning the transformed state. This mixing of side effects with state transformation makes the code harder to reason about and could lead to subtle bugs.♻️ Suggested refactor
Separate the data collection from the state update:
+ val currentSuccessState = lucideRecord.value.safeAs<LucideState.Success>() + currentSuccessState?.loadedIcons?.keys?.forEach { cacheKey -> + extractIconNameFromCacheKey(cacheKey)?.let { iconName -> + currentSuccessState.config.iconsByName[iconName]?.let { icon -> + iconsToReload.add(icon to cacheKey) + oldCacheKeys.add(cacheKey) + } + } + } updateSuccess { state -> - state.loadedIcons.keys.forEach { cacheKey -> - extractIconNameFromCacheKey(cacheKey)?.let { iconName -> - state.config.iconsByName[iconName]?.let { icon -> - iconsToReload.add(icon to cacheKey) - oldCacheKeys.add(cacheKey) - } - } - } - state.copy(settings = settings) }
180-202: Same stale state issue asloadIconForDisplay.Line 186 captures the current state, but by the time the check on line 187 executes, the state may have changed. This is less critical here since it's just an optimization to avoid redundant work, but for consistency, consider fetching the current state at the point of check.
💡 Minor improvement
val job = viewModelScope.launch { - val currentState = lucideRecord.value.safeAs<LucideState.Success>() ?: return@launch - if (currentState.loadedIcons[cacheKey] is IconLoadState.Success) { + val loadedIcons = lucideRecord.value.safeAs<LucideState.Success>()?.loadedIcons ?: return@launch + if (loadedIcons[cacheKey] is IconLoadState.Success) { return@launch }
📜 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/lucide/LucideViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt
🧰 Additional context used
🧬 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)
🔇 Additional comments (5)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt (1)
92-95: Good use ofLocale.ROOTfor consistent formatting.The hex conversion correctly uses
Locale.ROOTto ensure consistent output regardless of system locale, and properly masks the alpha channel.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (4)
68-91: LGTM!The initialization logic correctly:
- Skips reloading if config is already in
Successstate (handles process restoration)- Sets up debounced search with a reasonable 300ms delay
- Uses
Dispatchers.Defaultappropriately for CPU-bound filtering work
297-309: LGTM!The cache key construction using
||delimiter is well-documented and the extraction logic correctly handles it. The key includes all relevant settings properties that affect icon rendering.
358-377: Good design for theiconNameIndexlookup.The
iconNameIndexprovides an efficient way to retrieve the most recent successful render for an icon by name, independent of settings. This enables showing the last known good state while a re-render with new settings is in progress—a good UX pattern.The
@Stableannotations are appropriate for Compose performance optimization.
47-47: No action needed.LruCacheis thread-safe per Android documentation, and all cache operations (get,put,remove) are atomic. The code properly uses per-icon mutexes to coordinate the loading pipeline without requiring additional cache-level synchronization.Likely an incorrect or invalid review comment.
| 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() | ||
| rawSvgCache.put(iconName, downloaded) | ||
| downloaded | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Mutex held during network I/O blocks all concurrent cache access.
The mutex is held for the entire operation including the network download. This means if one coroutine is downloading an SVG, all other coroutines attempting to access the cache (even for already-cached icons) will be blocked. This can significantly degrade performance when loading multiple icons concurrently.
Consider using a double-check locking pattern: check cache, release lock, download, re-acquire lock, and store only if still absent.
🔧 Proposed fix using double-check locking
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()
- rawSvgCache.put(iconName, downloaded)
- downloaded
- }
+ // First check: return cached value without blocking download
+ cacheMutex.withLock { rawSvgCache[iconName] }?.let { return@withContext it }
+
+ // Download outside the lock
+ val url = "$UNPKG_BASE/icons/$iconName.svg"
+ val downloaded = httpClient.get(url).bodyAsText()
+
+ // Second check: store only if still absent (another coroutine may have cached it)
+ cacheMutex.withLock {
+ rawSvgCache[iconName] ?: run {
+ rawSvgCache.put(iconName, downloaded)
+ downloaded
+ }
}
}🤖 Prompt for AI Agents
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt
around lines 55 - 64, In getRawSvg, avoid holding cacheMutex across network I/O:
first acquire cacheMutex and check rawSvgCache[iconName]; if present return it
and release the lock; if absent release the lock, perform the
httpClient.get(...) download from "$UNPKG_BASE/icons/$iconName.svg", then
re-acquire cacheMutex and put the downloaded value into rawSvgCache only if the
entry is still missing (double-check) before returning; reference cacheMutex,
rawSvgCache, getRawSvg, httpClient and UNPKG_BASE to locate the changes.
| }.onFailure { error -> | ||
| LOG.error("Failed to download icon '${icon.name}'", error) | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent failure on icon download provides no user feedback.
When downloadIcon fails, the error is only logged. The user receives no indication that the download failed—they click an icon and nothing happens.
Consider emitting an error event so the UI can display a toast or error state.
💡 Suggested enhancement
}.onFailure { error ->
LOG.error("Failed to download icon '${icon.name}'", error)
+ _events.emit(LucideEvent.DownloadError(icon.name, error.message ?: "Unknown error"))
}And extend the event type:
sealed interface LucideEvent {
data class IconDownloaded(...) : LucideEvent
+ data class DownloadError(val iconName: String, val message: String) : LucideEvent
}There was a problem hiding this comment.
will be added later, for now behaviour matches existing MaterialIcons
There was a problem hiding this comment.
@t-regbs, understood! It makes sense to maintain consistency with the existing MaterialIcons implementation for now. Feel free to address the user feedback enhancement across both implementations in a future iteration when you're ready to improve the error handling experience for all web import features.
(ᵔᴥᵔ)
| fun loadIconForDisplay(icon: LucideIcon): Job { | ||
| val state = lucideRecord.value.safeAs<LucideState.Success>() ?: return Job() | ||
| val cacheKey = buildIconCacheKey(icon.name, state.settings) | ||
| iconLoadJobs[cacheKey]?.cancel() | ||
|
|
||
| val job = viewModelScope.launch { | ||
| val mutex = iconLoadMutexes.computeIfAbsent(cacheKey) { Mutex() } | ||
|
|
||
| mutex.withLock { | ||
| val currentState = state.loadedIcons[cacheKey] | ||
| if (currentState is IconLoadState.Success || currentState is IconLoadState.Loading) { | ||
| return@launch | ||
| } | ||
|
|
||
| iconVectorCache.get(cacheKey)?.imageVector?.let { cachedVector -> | ||
| val successState = IconLoadState.Success(cachedVector) | ||
| updateSuccess { | ||
| it.copy( | ||
| loadedIcons = it.loadedIcons + (cacheKey to successState), | ||
| iconNameIndex = it.iconNameIndex + (icon.name to successState), | ||
| ) | ||
| } | ||
| return@launch | ||
| } | ||
|
|
||
| updateSuccess { | ||
| it.copy(loadedIcons = it.loadedIcons + (cacheKey to IconLoadState.Loading)) | ||
| } | ||
| } | ||
|
|
||
| runCatching { | ||
| val rawSvg = lucideUseCase.getRawSvg(icon.name) | ||
| val customizedSvg = lucideUseCase.applyCustomizations(rawSvg, state.settings) | ||
|
|
||
| parseAndCacheIcon(icon.name, customizedSvg, cacheKey) | ||
| }.onFailure { error -> | ||
| handleIconParseError(icon.name, cacheKey, error) | ||
| } | ||
| } |
There was a problem hiding this comment.
Stale state capture leads to potential race conditions.
The state variable captured on line 213 becomes stale as other coroutines may modify lucideRecord.value. Two issues:
-
Line 221: The check
state.loadedIcons[cacheKey]uses the captured (old) state, not the current state. If another coroutine completed loading between lines 213 and 221, this won't detect it. -
Line 244: Uses
state.settingswhich may be outdated ifupdateSettingswas called during the network fetch, potentially applying wrong customizations.
🔧 Proposed fix
fun loadIconForDisplay(icon: LucideIcon): Job {
- val state = lucideRecord.value.safeAs<LucideState.Success>() ?: return Job()
- val cacheKey = buildIconCacheKey(icon.name, state.settings)
+ val initialState = lucideRecord.value.safeAs<LucideState.Success>() ?: return Job()
+ val cacheKey = buildIconCacheKey(icon.name, initialState.settings)
iconLoadJobs[cacheKey]?.cancel()
val job = viewModelScope.launch {
val mutex = iconLoadMutexes.computeIfAbsent(cacheKey) { Mutex() }
mutex.withLock {
- val currentState = state.loadedIcons[cacheKey]
+ val currentState = lucideRecord.value.safeAs<LucideState.Success>()
+ ?.loadedIcons?.get(cacheKey)
if (currentState is IconLoadState.Success || currentState is IconLoadState.Loading) {
return@launch
}
// ... cache check ...
}
runCatching {
+ val latestState = lucideRecord.value.safeAs<LucideState.Success>() ?: return@launch
val rawSvg = lucideUseCase.getRawSvg(icon.name)
- val customizedSvg = lucideUseCase.applyCustomizations(rawSvg, state.settings)
+ val customizedSvg = lucideUseCase.applyCustomizations(rawSvg, latestState.settings)
parseAndCacheIcon(icon.name, customizedSvg, cacheKey)
}
// ...
}| private inline fun updateSuccess(crossinline transform: (LucideState.Success) -> LucideState.Success) { | ||
| val current = lucideRecord.value | ||
| if (current is LucideState.Success) { | ||
| lucideRecord.value = transform(current) | ||
| } | ||
| } |
There was a problem hiding this comment.
Non-atomic read-modify-write in updateSuccess can cause lost updates.
This function reads lucideRecord.value, transforms it, and writes back. When multiple coroutines call this concurrently (e.g., two icons finish parsing at the same time), both read the same state and write their updates—one update gets lost.
Example scenario:
- Icon A finishes parsing, reads
loadedIcons = {} - Icon B finishes parsing, reads
loadedIcons = {}(same state) - Icon A writes
loadedIcons = {A: Success} - Icon B writes
loadedIcons = {B: Success}→ Icon A's update is lost
🔧 Proposed fix using atomic update
-private inline fun updateSuccess(crossinline transform: (LucideState.Success) -> LucideState.Success) {
- val current = lucideRecord.value
- if (current is LucideState.Success) {
- lucideRecord.value = transform(current)
- }
-}
+private val stateMutex = Mutex()
+
+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 requires making updateSuccess a suspend function, which will require updates to call sites. Alternatively, consider using MutableStateFlow.update { } which provides atomic updates if the underlying state holder supports it.
🤖 Prompt for AI Agents
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt
around lines 337 - 342, updateSuccess performs a non-atomic read-modify-write on
lucideRecord.value which can lose concurrent updates; change the implementation
to perform an atomic update (e.g., use MutableStateFlow.update { current -> if
(current is LucideState.Success) transform(current) else current } or implement
a compare-and-set loop) so multiple coroutines merging parsed icons won't
overwrite each other; make updateSuccess suspend if you choose an atomic
suspendable approach or convert lucideRecord to a MutableStateFlow and call its
update { } helper and update all call sites accordingly.
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.