Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Bootstrap colored ImageVector and accessor. Introduces a provider-based web import architecture: new StandardIconProvider interface, StandardIcon models/config, InferredCategory, SizeSettings and IconSettings, CategoryInferrer, SvgSizeCustomizer, StandardGridFiltering, and font conversion utility. Adds Bootstrap and Lucide provider implementations, repositories, use cases, DI modules, and import screens (BootstrapImportScreen, LucideImportScreen, StandardImportScreen). Replaces Lucide-specific UI and domain types with standardized equivalents, adds customization UI (CustomizationToolbar, IconSizeCustomization), and removes the legacy LucideUseCase and related Lucide domain models. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.kt`:
- Around line 35-43: The KDoc for IconSizeCustomization.kt contains a stale
example key "web.import.font.customize.lucide.size" for the sizeLabel parameter;
update the KDoc in the IconSizeCustomization file to use the new key
"web.import.font.customize.size" (update the `@param` sizeLabel example text) so
the documentation matches the renamed resource key and ensure the comment
mentions a generic example rather than the Lucide-specific one.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/CategoryInferrer.kt`:
- Around line 20-52: The "key" entry in CATEGORY_KEYWORDS
(CategoryKeyword("key", "Security", priority = 1)) causes substring
false-positives (e.g., "keyboard"); change the keyword to a more specific token
and/or add delimiter-aware matching: replace the CategoryKeyword("key", ...)
with a more precise keyword like "key-" or "padlock" (and/or "lock") and update
the matching code that consumes CATEGORY_KEYWORDS to perform word-boundary or
delimiter (e.g., '-' or '_') based matching instead of raw substring checks so
only exact token matches map to "Security".
🧹 Nitpick comments (9)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kt (2)
55-57: Consider sanitizing or validatingiconNamebefore URL interpolation.While
iconNamevalues originate from the parsed JSON keys (so they should be safe in practice), directly interpolating user-selectable strings into URLs without validation is a defensive coding concern. A simple check (e.g., verifying the name matches[a-z0-9-]+) would guard against unexpected input.
20-25: Consider pinningbootstrap-iconsto a specific version for reproducible builds.
UNPKG_BASEuses@latest, making the plugin's behavior non-deterministic—if Bootstrap Icons publishes a breaking change, the plugin could break without any code change on your side. However, this pattern is consistent withLucideRepository, which also uses@latestfor lucide-static. If pinning is desired, apply it across all icon repositories and use the current version (1.13.1 rather than 1.11.3).tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/CustomizationToolbar.kt (1)
28-38: Consider renaming string resource keys to remove "font" prefix.The toolbar is reused in both
FontCustomizationandIconSizeCustomizationcontexts, but the resource keysweb.import.font.customize.headerandweb.import.font.customize.resetcontain "font" in their names. While the displayed text ("Customize", "Reset") is already generic and functions correctly in both contexts, renaming these keys toweb.import.customize.headerandweb.import.customize.resetwould improve clarity for future maintainers.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kt (1)
25-33: Category inference relies solely on icon name for Bootstrap.Since Bootstrap icons lack tag metadata (
tags = emptyList()),inferCategoryFromTagswill only match on icon names. This means many Bootstrap icons may fall into the "General" fallback category if their names don't contain keywords fromCATEGORY_KEYWORDS. This may result in a less useful category filter compared to Lucide.Consider whether this is acceptable UX or whether Bootstrap-specific keyword tuning (or disabling category filtering for Bootstrap) would be worthwhile.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/CategoryInferrer.kt (1)
73-90: Priority boosting for name matches can produce negative priorities.Line 79:
it.copy(priority = it.priority - 2)means priority-1 keywords matched by name become priority -1. This works correctly since lower = higher priority, but the negative values and the implicit coupling between the boost value (-2) and the priority scale (1–5) could be fragile if new priority levels are added.A minor readability improvement would be to document the effective priority range (or use a named constant for the boost).
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardIconViewModel.kt (1)
154-159: Minor:updateSuccesshas a potential read-modify-write race.
selectCategory,updateSearchQuery, andupdateSettingsall dispatch onDispatchers.Defaultand callupdateSuccessconcurrently. Two concurrent calls can read the same snapshot and one update gets lost. In practice, UI-driven actions are near-sequential so the risk is low, but worth noting.A
Mutexor funneling all updates through a single-threaded dispatcher (e.g.,Dispatchers.Main) would eliminate the race.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt (2)
113-117: Modifier ordering:fillMaxSize()before externalmodifier.The external
modifieris applied afterfillMaxSize(), which means caller-supplied size constraints are ignored. If this is intentional (screen must always fill), consider documenting it; otherwise, swap the order.♻️ Suggested fix (if caller sizing should be respected)
- modifier = Modifier - .fillMaxSize() - .then(modifier), + modifier = modifier + .fillMaxSize(),
297-301: Unchecked cast:item.icon as StandardIcon.
IconItem<*>is erased at runtime, so this cast is unchecked. It works as long as onlyStandardIconitems are added togridItems, but a future misuse could produce aClassCastExceptionwith no compile-time guard.Consider using a typed
IconItem<StandardIcon>alias or a safe-cast with a fallback.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/LucideImportScreen.kt (1)
20-28:navController.parent?.navigatesilently no-ops if parent is null.If
parentis evernull, the download completes but the user sees no navigation — a silent failure. This is likely safe given the screen is always nested insideWebImportFlow, but a log or assertion would make debugging easier if the hierarchy changes.
...otlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.kt
Show resolved
Hide resolved
...tlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/CategoryInferrer.kt
Show resolved
Hide resolved
b28d2ae to
b69ae5d
Compare
| ) { | ||
| companion object { | ||
| private const val UNPKG_BASE = "https://unpkg.com/bootstrap-icons@latest" | ||
| private const val FONT_URL = "$UNPKG_BASE/font/fonts/bootstrap-icons.woff2" |
There was a problem hiding this comment.
Yeah it does, I wasn't sure myself at first. But this package only has a woff2 and woff, no .ttf. It works fine and I can't think of any issues with it.
Untitled.mov
There was a problem hiding this comment.
Wow, in this case, we can switch to woff2 for all icon providers. woff2 font files are half the size of ttf files. Will add a ticket
There was a problem hiding this comment.
@egorikftp Can't find reliable alternative source with ttf font for this. Think we can either:
- Download the WOFF2 from CDN, convert it to TTF, bundle the TTF with the plugin.
Just that this is a manual process, need to redo when Bootstrap Icons updates - Use SVG rendering instead of font icons
- I found Brotli4j, we can use this and write custom woff/woff2 to ttf parser in the plugin
Will prototype the third option first on a separate branch to see how feasible it is
There was a problem hiding this comment.
one more idea to use native bindings 💀
For example https://github.com/khoben/woff2-android but only for desktop platforms
...kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/CustomizationToolbar.kt
Show resolved
Hide resolved
...github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/BootstrapImportScreen.kt
Outdated
Show resolved
Hide resolved
...in/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/LucideImportScreen.kt
Outdated
Show resolved
Hide resolved
b69ae5d to
d93bbad
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/standard/bootstrap/BootstrapImportScreen.kt`:
- Around line 20-28: The current onIconDownload uses
navController.parent?.navigate which silently drops navigation when parent is
null; update BootstrapImportScreen's onIconDownload to handle a null parent
explicitly by either using a non-null assertion (e.g.,
requireNotNull/navController.parent!!) to fail fast during development or by
checking navController.parent and logging/showing an error before returning,
then calling navigate(dest = SimpleConversionScreen, navArgs = TextSource(...))
on the non-null parent to ensure the user gets feedback instead of a silent
no-op.
🧹 Nitpick comments (4)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/CategoryInferrer.kt (1)
73-90: Duplicate name+tag matches are harmless but create unnecessary allocations.If a keyword (e.g.,
"arrow") matches both the icon name and a tag, it appears twice inallMatches— once boosted, once at original priority.minByOrNullstill picks the correct (boosted) entry, so correctness is fine. The extra allocation is negligible for the expected dataset sizes.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardIconViewModel.kt (1)
154-159: Non-atomic read-modify-write onstateRecord.value.
updateSuccessreads the current value, checks the type, and writes back. If two coroutines call this concurrently (e.g.,selectCategoryonDispatchers.DefaultanddownloadFontcompleting simultaneously), the second write can silently overwrite changes from the first. The practical risk is low since user interactions are sequential, but be aware this is inherently racy.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt (2)
113-116: Modifier ordering:fillMaxSize()is applied before the caller'smodifier.The external
modifieris applied via.then(modifier)after.fillMaxSize(), which means callers cannot override the size constraint. Typically, the incoming modifier is applied first. If the intent is to always fill max size, consider documenting this or reordering.🔧 Suggested reorder
Column( - modifier = Modifier - .fillMaxSize() - .then(modifier), + modifier = modifier + .fillMaxSize(), ) {
297-303: Unchecked castitem.icon as StandardIcon.
IconItem<*>erases the type parameter, so this cast is unchecked and will only fail at runtime if a non-StandardIconitem is ever added to the grid. This is safe today since the ViewModel only suppliesStandardIconitems, but it's fragile.Consider using a typed alias (e.g.,
typealias StandardIconItem = IconItem<StandardIcon>) and filtering/casting at the data boundary instead of in the UI layer.
...github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/BootstrapImportScreen.kt
Show resolved
Hide resolved
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/util/font/WoffToTtfConverter.kt`:
- Around line 127-129: The loop in WoffToTtfConverter that repeatedly calls
inflater.inflate(buffer) can spin forever if inflate() returns 0 (e.g.,
truncated stream or dictionary required); change the loop to capture the
returned int (e.g., int read = inflater.inflate(buffer)) and if read == 0 then
detect the cause: if inflater.needsDictionary() throw an IOException indicating
a missing preset dictionary, if inflater.needsInput() either feed more input or
throw an IOException for truncated input, otherwise break/throw to avoid an
infinite loop; keep the rest of the inflater.finished() handling intact so you
still write non-zero reads to output.
🧹 Nitpick comments (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/WoffToTtfConverter.kt (1)
149-149: Consider using an integer-basedlog2instead of floating-pointln.Since the argument is always an exact power of 2 (from
highestPowerOf2),Integer.numberOfTrailingZerosis exact and avoids any floating-point edge cases.Proposed fix
- private fun log2(n: Int): Int = (ln(n.toDouble()) / ln(2.0)).toInt() + private fun log2(n: Int): Int = Integer.numberOfTrailingZeros(n)This also removes the
import kotlin.math.lnon line 8.
.../idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/WoffToTtfConverter.kt
Outdated
Show resolved
Hide resolved
… instead of woff converter
…l command for windows
|
@egorikftp Been exploring the JNI binding idea and I like it. See latests commits on this PR, I added:
Let me know your thoughts |
Uh oh!
There was an error while loading. Please reload this page.