[Plugin] Add Bootstrap Icons and woff2 to ttf native decoder#858
Merged
[Plugin] Add Bootstrap Icons and woff2 to ttf native decoder#858
Conversation
WalkthroughAdds a provider-based "standard" web-import architecture and Bootstrap support. Introduces StandardIconProvider and implementations for Bootstrap and Lucide, new models (StandardIcon, StandardIconConfig, SizeSettings, InferredCategory), category inference and display-name utilities, and UI components (CustomizationToolbar, IconSizeCustomization, StandardImport screens and destinations). Adds a colored SDK icon BootstrapLogo. Removes legacy Lucide domain files and migrates many lucide paths to the standard layout. Adds native WOFF2 decoding (JNI C++ bridge, NativeLibraryLoader, Woff2Decoder Kotlin) and a GitHub Actions workflow to build native libraries. Localization keys updated. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
t-regbs
force-pushed
the
feature/bootstrap-icons
branch
from
b28d2ae to
b69ae5d
Compare
egorikftp
reviewed
Feb 13, 2026
...hub/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kt
Show resolved
Hide resolved
egorikftp
reviewed
Feb 13, 2026
...kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/CustomizationToolbar.kt
Show resolved
Hide resolved
egorikftp
reviewed
Feb 13, 2026
...github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/BootstrapImportScreen.kt
Outdated
Show resolved
Hide resolved
egorikftp
reviewed
Feb 13, 2026
...in/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/LucideImportScreen.kt
Outdated
Show resolved
Hide resolved
t-regbs
force-pushed
the
feature/bootstrap-icons
branch
from
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
Collaborator
Author
|
@egorikftp Been exploring the JNI binding idea and I like it. See latests commits on this PR, I added:
Let me know your thoughts |
Member
Amazing work you’ve done! I’ll test it on all platforms and get back with feedback ❤️ |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/idea-plugin/native/woff2decoder.cpp`:
- Around line 11-33: Get and check JNI allocations and exceptions: after calling
env->GetByteArrayElements (input_data) validate input_data != nullptr and call
env->ExceptionCheck() and return nullptr if either indicates failure; use
env->GetArrayLength only after confirming input_data is valid. After creating
the output jbyteArray via env->NewByteArray check for nullptr and
env->ExceptionCheck() and if failed release the input bytes with
env->ReleaseByteArrayElements(..., JNI_ABORT) and return appropriately; never
call env->SetByteArrayRegion or further JNI calls when an exception is pending.
Replace ReleaseByteArrayElements(..., 0) with JNI_ABORT for the read-only input
buffer and ensure any cleanup on failure uses the same JNI_ABORT flag; reference
functions/objects: GetByteArrayElements, NewByteArray, ReleaseByteArrayElements,
SetByteArrayRegion, ConvertWOFF2ToTTF, WOFF2StringOut and out.Size() to locate
where to add these checks and early returns.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kt`:
- Around line 31-40: The current extraction writes the native library into a
predictable temp directory and fixed filename (tempDir / tempFile) which risks
collisions and hijacking; update NativeLibraryLoader.kt to create a unique,
secure temporary directory and file for each load (use Files.createTempDirectory
or File.createTempFile semantics), write the resourceStream into that temp file
with restrictive permissions, atomically move if needed, call deleteOnExit, and
ensure the code paths around tempDir/tempFile (the variables and the
resourceStream.use block) reference the new unique temp path so each load uses
its own non-predictable file.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tools/idea-plugin/src/main/resources/native/linux-x64/libwoff2decoder.sois excluded by!**/*.sotools/idea-plugin/src/main/resources/native/macos/libwoff2decoder.dylibis excluded by!**/*.dylibtools/idea-plugin/src/main/resources/native/windows-x64/woff2decoder.dllis excluded by!**/*.dll
📒 Files selected for processing (5)
.github/workflows/build-native-libs.ymltools/idea-plugin/native/woff2decoder.cpptools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/Woff2Decoder.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/standard/bootstrap/data/BootstrapRepository.kt
...idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tools/idea-plugin/native/woff2decoder.cpp (1)
11-33: JNI allocation error handling is missing.This issue was flagged in a previous review. The code lacks null checks after
GetByteArrayElements(line 11) andNewByteArray(line 29), and doesn't check for pending exceptions. Additionally,ReleaseByteArrayElementsshould useJNI_ABORTinstead of0for read-only buffers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/native/woff2decoder.cpp` around lines 11 - 33, Get and use JNI resources defensively: after calling env->GetByteArrayElements (input_bytes) check for a null return and env->ExceptionCheck() and return nullptr if allocation/attachment failed; after creating the Java output buffer with env->NewByteArray check for null and pending exceptions and clean up (release input) before returning; always call env->ReleaseByteArrayElements(input_bytes, ...) with JNI_ABORT (not 0) since we only read from the input; ensure any early returns release resources and propagate failure by returning nullptr. Target these changes around GetByteArrayElements, GetArrayLength, NewByteArray, and ReleaseByteArrayElements in woff2decoder.cpp and add appropriate env->ExceptionCheck() checks and cleanup paths.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kt (1)
31-40: Predictable temp path creates security and concurrency risks.This was flagged in a previous review. The fixed path
valkyrie-native/$fileNameallows collisions between JVM instances and potential symlink attacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kt` around lines 31 - 40, The code in NativeLibraryLoader creates a predictable temp path (tempDir = File(System.getProperty("java.io.tmpdir"), "valkyrie-native") and tempFile = File(tempDir, fileName), which risks collisions and symlink attacks; change to create a secure, unique temp location using the platform temp APIs (e.g., Files.createTempDirectory or Files.createTempFile) and write the resource into that securely-created file, set restrictive permissions, and call deleteOnExit; update symbols: tempDir, tempFile, fileName, resourceStream, and the code path in NativeLibraryLoader that copies the stream so it uses the unique temp file instead of the fixed "valkyrie-native/$fileName" path.
🧹 Nitpick comments (2)
.github/workflows/build-native-libs.yml (2)
57-61: Consider adding artifact retention and a consolidation job.The artifacts are uploaded separately without retention configuration. Consider:
- Adding
retention-daysto control artifact storage- Adding a final job that downloads all artifacts and uploads a combined release artifact for easier consumption
Also applies to: 129-133, 218-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-native-libs.yml around lines 57 - 61, Update each actions/upload-artifact@v4 step (the "Upload artifact" step blocks) to include a retention-days input (e.g., retention-days: 30) to control artifact storage, and add a new final job (e.g., "consolidate-artifacts") that depends on the build jobs, uses actions/download-artifact to fetch each uploaded artifact by name, consolidates them into a single archive, and then uploads that archive with actions/upload-artifact (or creates a release) for easier consumption; apply the retention-days change to the other upload steps referenced (lines 129-133 and 218-222) as well.
18-19: Pin the woff2 repository to a specific commit or tag for reproducible builds.Cloning
google/woff2without a version pin means builds could break or produce different binaries if upstream changes. Pin to a specific commit or release tag for reproducibility.- - name: Clone google/woff2 - run: git clone --recursive --depth 1 https://github.com/google/woff2.git /tmp/woff2 + - name: Clone google/woff2 + run: | + git clone --recursive --depth 1 https://github.com/google/woff2.git /tmp/woff2 + cd /tmp/woff2 && git checkout v1.0.2 # or specific commit SHAAlso applies to: 77-78, 146-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-native-libs.yml around lines 18 - 19, The workflow clones google/woff2 without pinning, making builds unreproducible; update each git clone step that runs "git clone --recursive --depth 1 https://github.com/google/woff2.git /tmp/woff2" (the "Clone google/woff2" steps) to check out a specific commit or tag by either using a branch/tag ref (e.g., add "--branch <tag-or-commit>" or clone then run "git checkout <commit-or-tag>") or appending "@<commit>" in the URL, ensuring all occurrences (the three clone steps) are changed to the same pinned commit/tag for reproducible builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kt`:
- Around line 62-66: The parseCodepoints function is treating Bootstrap
codepoint strings as decimal; update parseCodepoints (in BootstrapRepository) to
parse values as hexadecimal by calling toIntOrNull(16), first trimming any "0x"
prefix and whitespace from value.jsonPrimitive.content; ensure the map still
filters nulls the same way so downstream code that constructs Chars from
icon.codepoint receives correct integer codepoints.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kt`:
- Around line 31-32: The code currently calls tempDir.mkdirs() without checking
its return; update the NativeLibraryLoader logic to verify the directory exists
after attempting creation by checking tempDir.exists() or tempDir.isDirectory(),
and if mkdirs() returned false and the directory does not exist, throw or log a
descriptive exception (including the path in tempDir) so the failure is explicit
before any file writes; reference the tempDir variable and the mkdirs() call in
NativeLibraryLoader.kt when making this change.
---
Duplicate comments:
In `@tools/idea-plugin/native/woff2decoder.cpp`:
- Around line 11-33: Get and use JNI resources defensively: after calling
env->GetByteArrayElements (input_bytes) check for a null return and
env->ExceptionCheck() and return nullptr if allocation/attachment failed; after
creating the Java output buffer with env->NewByteArray check for null and
pending exceptions and clean up (release input) before returning; always call
env->ReleaseByteArrayElements(input_bytes, ...) with JNI_ABORT (not 0) since we
only read from the input; ensure any early returns release resources and
propagate failure by returning nullptr. Target these changes around
GetByteArrayElements, GetArrayLength, NewByteArray, and ReleaseByteArrayElements
in woff2decoder.cpp and add appropriate env->ExceptionCheck() checks and cleanup
paths.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kt`:
- Around line 31-40: The code in NativeLibraryLoader creates a predictable temp
path (tempDir = File(System.getProperty("java.io.tmpdir"), "valkyrie-native")
and tempFile = File(tempDir, fileName), which risks collisions and symlink
attacks; change to create a secure, unique temp location using the platform temp
APIs (e.g., Files.createTempDirectory or Files.createTempFile) and write the
resource into that securely-created file, set restrictive permissions, and call
deleteOnExit; update symbols: tempDir, tempFile, fileName, resourceStream, and
the code path in NativeLibraryLoader that copies the stream so it uses the
unique temp file instead of the fixed "valkyrie-native/$fileName" path.
---
Nitpick comments:
In @.github/workflows/build-native-libs.yml:
- Around line 57-61: Update each actions/upload-artifact@v4 step (the "Upload
artifact" step blocks) to include a retention-days input (e.g., retention-days:
30) to control artifact storage, and add a new final job (e.g.,
"consolidate-artifacts") that depends on the build jobs, uses
actions/download-artifact to fetch each uploaded artifact by name, consolidates
them into a single archive, and then uploads that archive with
actions/upload-artifact (or creates a release) for easier consumption; apply the
retention-days change to the other upload steps referenced (lines 129-133 and
218-222) as well.
- Around line 18-19: The workflow clones google/woff2 without pinning, making
builds unreproducible; update each git clone step that runs "git clone
--recursive --depth 1 https://github.com/google/woff2.git /tmp/woff2" (the
"Clone google/woff2" steps) to check out a specific commit or tag by either
using a branch/tag ref (e.g., add "--branch <tag-or-commit>" or clone then run
"git checkout <commit-or-tag>") or appending "@<commit>" in the URL, ensuring
all occurrences (the three clone steps) are changed to the same pinned
commit/tag for reproducible builds.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tools/idea-plugin/src/main/resources/native/linux-x64/libwoff2decoder.sois excluded by!**/*.sotools/idea-plugin/src/main/resources/native/macos/libwoff2decoder.dylibis excluded by!**/*.dylibtools/idea-plugin/src/main/resources/native/windows-x64/woff2decoder.dllis excluded by!**/*.dll
📒 Files selected for processing (5)
.github/workflows/build-native-libs.ymltools/idea-plugin/native/woff2decoder.cpptools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/Woff2Decoder.kt
...hub/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kt
Show resolved
Hide resolved
...idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kt
Outdated
Show resolved
Hide resolved
Member
|
@t-regbs We verified on different platforms (also migrated all icon providers to use woff2 + conversion) and everything is working without crash. Let's process with this solution! |
t-regbs
force-pushed
the
feature/bootstrap-icons
branch
from
9a84ca1 to
7b4e36f
Compare
There was a problem hiding this comment.
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/CHANGELOG.md (1)
32-39:⚠️ Potential issue | 🟡 MinorChangelog entry appears under a past release section.
Line 38 adds the Bootstrap provider under 1.1.0 – 2026-02-13, which is already in the past (today is 2026-02-24). If this change is for the upcoming release, move it to Unreleased; otherwise please confirm it’s a backport.
✏️ Suggested changelog placement
@@ ### Added @@ +- Add `Bootstrap` icon provider into Web import feature @@ ## 1.1.0 - 2026-02-13 ### Added @@ -- Add `Bootstrap` icon provider into Web import feature🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/CHANGELOG.md` around lines 32 - 39, The changelog entry adding the Bootstrap icon provider is placed under the past release header "## 1.1.0 - 2026-02-13" but appears to be for an upcoming change; move the bullet "- Add `Bootstrap` icon provider into Web import feature" out of the "## 1.1.0 - 2026-02-13" section and place it under the "## Unreleased" section (or create an "## Unreleased" section at the top if missing), or if this is intentionally a backport, annotate the entry to indicate it is a backport to 1.1.0 (e.g., add "(backport)" after the bullet) so the placement is explicit; update only the CHANGELOG.md contents accordingly.
🧹 Nitpick comments (9)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/domain/model/font/FontSettings.kt (1)
18-19: Consider includingiconFontFamilyinisModified.Line 18–19 currently ignores
iconFontFamily, so changing the family won’t enable Reset. If family selection is user-editable, add it to the predicate (or document why it’s intentionally excluded).Proposed tweak
- get() = fill || weight != 400 || grade != 0 || opticalSize != 24f + get() = fill || weight != 400 || grade != 0 || opticalSize != 24f || + iconFontFamily != IconFontFamily.OUTLINED🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/domain/model/font/FontSettings.kt` around lines 18 - 19, The isModified getter in FontSettings currently checks fill, weight, grade, and opticalSize but omits iconFontFamily, so changes to the family won't enable the Reset state; update the override val isModified: Boolean getter to include a comparison against the default iconFontFamily (e.g., add "|| iconFontFamily != <defaultValue>") or, if iconFontFamily is nullable/use a specific default constant, compare appropriately (reference isModified and iconFontFamily in class FontSettings) so selecting a different family toggles modification; alternatively, document why iconFontFamily is intentionally excluded if that is desired behavior.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/data/LucideRepository.kt (1)
32-43:loadIconList()silently hits the network on every call — consider adding a cache.
loadFontBytes()andloadCodepoints()are both guarded by a mutex + nullable-cache pattern, butloadIconList()has no equivalent. If the caller invokes it on repeated screen entries or recompositions, it will perform redundant network round-trips totags.json.♻️ Suggested cache addition (consistent with existing pattern)
private val fontMutex = Mutex() private val codepointMutex = Mutex() +private val iconListMutex = Mutex() private var fontBytesCache: ByteArray? = null private var codepointsCache: Map<String, Int>? = null +private var iconListCache: List<Pair<String, LucideIconMetadata>>? = null- 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(), - ) - } - } + suspend fun loadIconList(): List<Pair<String, LucideIconMetadata>> = withContext(Dispatchers.IO) { + iconListMutex.withLock { + iconListCache ?: run { + val response = httpClient.get("$UNPKG_BASE/tags.json") + val tagsJson = json.parseToJsonElement(response.bodyAsText()) as JsonObject + val result = tagsJson.entries.map { (iconName, tagsArray) -> + val tags = tagsArray.jsonArray.map { it.jsonPrimitive.content } + iconName to LucideIconMetadata( + tags = tags, + categories = emptyList(), + ) + } + iconListCache = result + result + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/data/LucideRepository.kt` around lines 32 - 43, loadIconList() currently performs a network request on every call; add the same mutex + nullable-cache pattern used by loadFontBytes()/loadCodepoints() to avoid redundant requests. Introduce a private nullable cache (e.g., iconListCache: List<Pair<String, LucideIconMetadata>>?) and a Mutex (e.g., iconListMutex = Mutex()), then change loadIconList() to first run iconListMutex.withLock { if (iconListCache == null) { perform the current withContext(Dispatchers.IO) fetch/parse and assign iconListCache } return iconListCache!! } so the network is only hit once and subsequent callers return the cached list..github/workflows/build-native-libs.yml (4)
6-222: No smoke test to verify the built libraries are actually loadable.Each job verifies the output file exists (
file,ls -lh,Get-Item), but none checks that the library exports the expected JNI symbol. A quicknm/objdump/dumpbincheck would catch missing symbols or link errors early:# Linux nm -D libwoff2decoder.so | grep Java_io_github_composegears_valkyrie_util_font_Woff2Decoder_decodeBytes # macOS nm libwoff2decoder.dylib | grep Java_io_github_composegears_valkyrie_util_font_Woff2Decoder_decodeBytes # Windows (in MSVC shell) dumpbin /exports woff2decoder.dll | findstr decodeBytes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-native-libs.yml around lines 6 - 222, Add a smoke-test step after each "Verify library" step that checks the JNI export Java_io_github_composegears_valkyrie_util_font_Woff2Decoder_decodeBytes is present and fails the job if not found: for Linux run nm -D against libwoff2decoder.so and grep the symbol; for macOS run nm against libwoff2decoder.dylib and grep the symbol; for Windows run dumpbin /exports against woff2decoder.dll and findstr the symbol; ensure the step uses the same output filenames used in Build JNI shared library / Create universal binary / Build JNI DLL to locate the files.
99-123: Windows library discovery is fragile.The
Get-ChildItem -Recurse -Filterapproach (lines 105–108) for locating.libfiles works but is sensitive to build output layout changes across CMake/MSVC versions. TheWhere-Objectfilter on line 107–108 matching'static'or an exact name could also miss renamed outputs. Consider logging a failure and exiting early if any library is not found:🛡️ Suggested guard
+ if (-not $woff2dec) { throw "woff2dec.lib not found" } + if (-not $woff2common) { throw "woff2common.lib not found" } + if (-not $brotlidec) { throw "brotlidec lib not found" } + if (-not $brotlicommon) { throw "brotlicommon lib not found" } + Write-Host "woff2dec: $($woff2dec.FullName)"Without this, if a library isn't found, the
clinvocation receives an empty argument and fails with a cryptic error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-native-libs.yml around lines 99 - 123, The PowerShell step currently uses $woff2dec, $woff2common, $brotlidec, and $brotlicommon and then calls cl with their FullName, but it doesn't check whether those Get-ChildItem lookups returned anything; if any is null cl will get an empty argument and fail cryptically. Add a guard after the lookups that checks each of $woff2dec, $woff2common, $brotlidec, $brotlicommon for null/empty, Write-Host/Write-Error with a clear message including which variable is missing and the search patterns used, and then exit the script early (non-zero) so the job fails fast; keep the existing diagnostic Write-Host lines and consider widening the matching logic or logging the paths you searched to aid debugging.
19-19: Thegoogle/woff2clone is unpinned — builds are not reproducible.
git clone --recursive --depth 1fetches whatever HEAD is at the time. If google/woff2 pushes a breaking change or introduces a bug, subsequent workflow runs will silently pick it up. Consider pinning to a specific commit or tag.♻️ Pin to a specific commit
- run: git clone --recursive --depth 1 https://github.com/google/woff2.git /tmp/woff2 + run: | + git clone --recursive https://github.com/google/woff2.git /tmp/woff2 + cd /tmp/woff2 && git checkout <known-good-commit-sha>This applies to lines 19, 78, and 147 (all three jobs).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-native-libs.yml at line 19, The git clone of google/woff2 uses an unpinned checkout ("git clone --recursive --depth 1 https://github.com/google/woff2.git /tmp/woff2") in three places; replace each unpinned clone with a reproducible checkout by cloning then checking out a specific commit or tag (e.g., run: git clone --recursive https://github.com/google/woff2.git /tmp/woff2 && cd /tmp/woff2 && git checkout <COMMIT_SHA> && git submodule update --init --recursive), or use --branch <TAG> --single-branch if you pin to a tag, ensuring the same commit is used every workflow run.
42-42: Inconsistent C++ standard: Linux usesc++11, Windows usesc++14.Line 42 (Linux) uses
-std=c++11and line 115 (Windows) uses/std:c++14. Line 169/199 (macOS) uses-std=c++11. While google/woff2 should compile with either, the inconsistency could mask platform-specific compilation issues. Consider aligning all three to the same standard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-native-libs.yml at line 42, The workflow uses mixed C++ standards: change all occurrences of the compiler standard flags so they are consistent (e.g., use C++14 everywhere); specifically replace the g++ flags "-std=c++11" (used in the Linux and macOS g++ command lines such as the g++ -shared -std=c++11 -O2 -fPIC invocation) with "-std=c++14", and update the Windows cl/MSVC flag "/std:c++14" if you choose a different standard so all three platforms use the same flag (either all "/std:c++14" for MSVC and "-std=c++14" for gcc/clang, or all on c++11 consistently) to ensure uniform compilation semantics across Linux, macOS and Windows.tools/idea-plugin/native/woff2decoder.cpp (1)
37-37: Narrowing cast fromsize_ttojsize(signed 32-bit).
out.Size()returnssize_t(up to 64-bit unsigned), whilejsizeisjint(signed 32-bit, max ~2 GB). The allocation is capped bywoff2::kDefaultMaxSize(256 MB), so this is safe in practice, but a defensive bounds check would make the intent explicit.🛡️ Optional defensive check
+ if (out.Size() > static_cast<size_t>(std::numeric_limits<jsize>::max())) { + fprintf(stderr, "woff2decoder: output too large for JNI array\n"); + env->ReleaseByteArrayElements(input_bytes, reinterpret_cast<jbyte *>(input_data), JNI_ABORT); + return nullptr; + } + jbyteArray arr = env->NewByteArray(static_cast<jsize>(out.Size()));(Requires
#include <limits>)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/native/woff2decoder.cpp` at line 37, The call env->NewByteArray(static_cast<jsize>(out.Size())) narrows a size_t to jsize; add a defensive bounds check before allocating: verify out.Size() <= std::min<std::size_t>(static_cast<std::size_t>(std::numeric_limits<jsize>::max()), woff2::kDefaultMaxSize) (include <limits>), and if it exceeds the limit, handle gracefully (e.g., throw a Java exception via env->ThrowNew or return nullptr) instead of performing the unsafe cast; keep the allocation using env->NewByteArray with a safe static_cast<jsize> after the check and reference out.Size(), env->NewByteArray, and woff2::kDefaultMaxSize in your change.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kt (1)
25-33: Consider wiring real tags when Bootstrap metadata becomes available.
tags = emptyList()is passed hardcoded, so category inference falls back to name-only matching. The Bootstrap Icons font package includes per-icon tag metadata (inbootstrap-icons.json). IfBootstrapRepository.loadIconList()is ever extended to expose those tags, substituting them here would noticeably improve category accuracy — especially for icons where the name alone is ambiguous.No change required now; just flagging as a natural follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kt` around lines 25 - 33, The current mapping that builds StandardIcon objects uses tags = emptyList(), which forces inferCategoryFromTags(name, emptyList()) to only use the name; when BootstrapRepository.loadIconList() is later extended to return per-icon tag metadata, replace the emptyList() usage in the codepoints.map block that constructs StandardIcon (the block creating StandardIcon with name/toDisplayName/codepoint) so that you pass the real tag list into both the StandardIcon.tags field and into inferCategoryFromTags(name, tags); this ensures category inference uses the real tags instead of falling back to name-only matching.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt (1)
281-303: Runtime cast onIconItemcontent is safe but unguarded.Line 300 casts
item.icontoStandardIconat runtime. SinceStandardIconGridisprivateand exclusively fed byStandardState.Success.gridItems(which always containsStandardIconitems), this cannot fail today. However, there's no compile-time guarantee — a future refactor that passes differently typedGridItems would surface as aClassCastExceptionat runtime rather than a build error.Consider a defensive cast to make the failure mode explicit:
♻️ Defensive cast proposal
- is IconItem<*> -> iconContent(item.icon as StandardIcon) + is IconItem<*> -> (item.icon as? StandardIcon)?.let { iconContent(it) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt` around lines 281 - 303, The code performs an unchecked cast item.icon as StandardIcon inside StandardIconGrid which can throw at runtime; update the when branch for IconItem to perform a safe cast (e.g., val icon = (item as IconItem<*>).icon as? StandardIcon) and only call iconContent(icon) when the cast succeeds (otherwise skip or render a fallback), or more strongly, change the gridItems typing so IconItem is IconItem<StandardIcon> to make the contract compile-time enforced; reference StandardIconGrid, GridItem, IconItem, iconContent and StandardIcon when applying the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tools/idea-plugin/CHANGELOG.md`:
- Around line 32-39: The changelog entry adding the Bootstrap icon provider is
placed under the past release header "## 1.1.0 - 2026-02-13" but appears to be
for an upcoming change; move the bullet "- Add `Bootstrap` icon provider into
Web import feature" out of the "## 1.1.0 - 2026-02-13" section and place it
under the "## Unreleased" section (or create an "## Unreleased" section at the
top if missing), or if this is intentionally a backport, annotate the entry to
indicate it is a backport to 1.1.0 (e.g., add "(backport)" after the bullet) so
the placement is explicit; update only the CHANGELOG.md contents accordingly.
---
Nitpick comments:
In @.github/workflows/build-native-libs.yml:
- Around line 6-222: Add a smoke-test step after each "Verify library" step that
checks the JNI export
Java_io_github_composegears_valkyrie_util_font_Woff2Decoder_decodeBytes is
present and fails the job if not found: for Linux run nm -D against
libwoff2decoder.so and grep the symbol; for macOS run nm against
libwoff2decoder.dylib and grep the symbol; for Windows run dumpbin /exports
against woff2decoder.dll and findstr the symbol; ensure the step uses the same
output filenames used in Build JNI shared library / Create universal binary /
Build JNI DLL to locate the files.
- Around line 99-123: The PowerShell step currently uses $woff2dec,
$woff2common, $brotlidec, and $brotlicommon and then calls cl with their
FullName, but it doesn't check whether those Get-ChildItem lookups returned
anything; if any is null cl will get an empty argument and fail cryptically. Add
a guard after the lookups that checks each of $woff2dec, $woff2common,
$brotlidec, $brotlicommon for null/empty, Write-Host/Write-Error with a clear
message including which variable is missing and the search patterns used, and
then exit the script early (non-zero) so the job fails fast; keep the existing
diagnostic Write-Host lines and consider widening the matching logic or logging
the paths you searched to aid debugging.
- Line 19: The git clone of google/woff2 uses an unpinned checkout ("git clone
--recursive --depth 1 https://github.com/google/woff2.git /tmp/woff2") in three
places; replace each unpinned clone with a reproducible checkout by cloning then
checking out a specific commit or tag (e.g., run: git clone --recursive
https://github.com/google/woff2.git /tmp/woff2 && cd /tmp/woff2 && git checkout
<COMMIT_SHA> && git submodule update --init --recursive), or use --branch <TAG>
--single-branch if you pin to a tag, ensuring the same commit is used every
workflow run.
- Line 42: The workflow uses mixed C++ standards: change all occurrences of the
compiler standard flags so they are consistent (e.g., use C++14 everywhere);
specifically replace the g++ flags "-std=c++11" (used in the Linux and macOS g++
command lines such as the g++ -shared -std=c++11 -O2 -fPIC invocation) with
"-std=c++14", and update the Windows cl/MSVC flag "/std:c++14" if you choose a
different standard so all three platforms use the same flag (either all
"/std:c++14" for MSVC and "-std=c++14" for gcc/clang, or all on c++11
consistently) to ensure uniform compilation semantics across Linux, macOS and
Windows.
In `@tools/idea-plugin/native/woff2decoder.cpp`:
- Line 37: The call env->NewByteArray(static_cast<jsize>(out.Size())) narrows a
size_t to jsize; add a defensive bounds check before allocating: verify
out.Size() <=
std::min<std::size_t>(static_cast<std::size_t>(std::numeric_limits<jsize>::max()),
woff2::kDefaultMaxSize) (include <limits>), and if it exceeds the limit, handle
gracefully (e.g., throw a Java exception via env->ThrowNew or return nullptr)
instead of performing the unsafe cast; keep the allocation using
env->NewByteArray with a safe static_cast<jsize> after the check and reference
out.Size(), env->NewByteArray, and woff2::kDefaultMaxSize in your change.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/domain/model/font/FontSettings.kt`:
- Around line 18-19: The isModified getter in FontSettings currently checks
fill, weight, grade, and opticalSize but omits iconFontFamily, so changes to the
family won't enable the Reset state; update the override val isModified: Boolean
getter to include a comparison against the default iconFontFamily (e.g., add "||
iconFontFamily != <defaultValue>") or, if iconFontFamily is nullable/use a
specific default constant, compare appropriately (reference isModified and
iconFontFamily in class FontSettings) so selecting a different family toggles
modification; alternatively, document why iconFontFamily is intentionally
excluded if that is desired behavior.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kt`:
- Around line 25-33: The current mapping that builds StandardIcon objects uses
tags = emptyList(), which forces inferCategoryFromTags(name, emptyList()) to
only use the name; when BootstrapRepository.loadIconList() is later extended to
return per-icon tag metadata, replace the emptyList() usage in the
codepoints.map block that constructs StandardIcon (the block creating
StandardIcon with name/toDisplayName/codepoint) so that you pass the real tag
list into both the StandardIcon.tags field and into inferCategoryFromTags(name,
tags); this ensures category inference uses the real tags instead of falling
back to name-only matching.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/data/LucideRepository.kt`:
- Around line 32-43: loadIconList() currently performs a network request on
every call; add the same mutex + nullable-cache pattern used by
loadFontBytes()/loadCodepoints() to avoid redundant requests. Introduce a
private nullable cache (e.g., iconListCache: List<Pair<String,
LucideIconMetadata>>?) and a Mutex (e.g., iconListMutex = Mutex()), then change
loadIconList() to first run iconListMutex.withLock { if (iconListCache == null)
{ perform the current withContext(Dispatchers.IO) fetch/parse and assign
iconListCache } return iconListCache!! } so the network is only hit once and
subsequent callers return the cached list.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt`:
- Around line 281-303: The code performs an unchecked cast item.icon as
StandardIcon inside StandardIconGrid which can throw at runtime; update the when
branch for IconItem to perform a safe cast (e.g., val icon = (item as
IconItem<*>).icon as? StandardIcon) and only call iconContent(icon) when the
cast succeeds (otherwise skip or render a fallback), or more strongly, change
the gridItems typing so IconItem is IconItem<StandardIcon> to make the contract
compile-time enforced; reference StandardIconGrid, GridItem, IconItem,
iconContent and StandardIcon when applying the fix.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tools/idea-plugin/src/main/resources/native/linux-x64/libwoff2decoder.sois excluded by!**/*.sotools/idea-plugin/src/main/resources/native/macos/libwoff2decoder.dylibis excluded by!**/*.dylibtools/idea-plugin/src/main/resources/native/windows-x64/woff2decoder.dllis excluded by!**/*.dll
📒 Files selected for processing (42)
.github/workflows/build-native-libs.ymlsdk/compose/icons/api/icons.apisdk/compose/icons/api/icons.klib.apisdk/compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/compose/icons/colored/BootstrapLogo.kttools/idea-plugin/CHANGELOG.mdtools/idea-plugin/native/woff2decoder.cpptools/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/model/IconSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/CustomizationToolbar.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.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/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/material/domain/model/font/FontSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/ui/FontCustomization.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardIconViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/BootstrapImportScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/di/BootstrapModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/CategoryInferrer.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/DisplayNameFormatter.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/StandardIconProvider.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/SvgSizeCustomizer.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/LucideImportScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/data/LucideIconMetadata.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/data/LucideRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/di/LucideModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/domain/LucideUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/InferredCategory.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/SizeSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/StandardIcon.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/StandardIconConfig.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/ui/StandardTopActions.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/util/StandardGridFiltering.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/Woff2Decoder.kttools/idea-plugin/src/main/resources/messages/Valkyrie.properties
💤 Files with no reviewable changes (5)
- 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/domain/LucideUseCase.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/lucide/domain/model/LucideConfig.kt
🚧 Files skipped from review as they are similar to previous changes (14)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/CustomizationToolbar.kt
- tools/idea-plugin/src/main/resources/messages/Valkyrie.properties
- sdk/compose/icons/api/icons.klib.api
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/util/StandardGridFiltering.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/StandardIconConfig.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/domain/LucideUseCase.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/standard/lucide/LucideImportScreen.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/di/LucideModule.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/StandardIcon.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/common/model/IconSettings.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/SizeSettings.kt
… instead of woff converter
…l command for windows
…s and improve cleanup; add null checks and exception handling in woff2decoder.cpp JNI functions
egorikftp
force-pushed
the
feature/bootstrap-icons
branch
from
7b4e36f to
bba0bd7
Compare
egorikftp
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kt (1)
62-68: Codepoint parsing — decimal format confirmed correct.As previously discussed, the Bootstrap Icons JSON from unpkg uses decimal integers (e.g.,
61697), sotoIntOrNull()without a radix is the correct approach here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kt` around lines 62 - 68, The parseCodepoints function correctly parses decimal codepoints using toIntOrNull(), so no code change is needed; instead resolve the duplicate review note by removing the duplicate comment marker or marking the earlier comment as addressed in the PR discussion and confirm parseCodepoints (function name parseCodepoints and usage of toIntOrNull on value.jsonPrimitive.content) remains unchanged.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/CategoryInferrer.kt (1)
20-52: "key" substring matching — already acknowledged.The
"key"keyword (Line 28) will match icon names like"keyboard", but this was previously discussed and accepted as best-effort categorization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/CategoryInferrer.kt` around lines 20 - 52, The CATEGORY_KEYWORDS list contains an entry CategoryKeyword("key", "Security", priority = 1) which will match substrings like "keyboard"; leave the keyword but make matching explicit to avoid unintended hits by changing the matching logic to prefer whole-token matches or boundary-aware checks: update the code that uses CATEGORY_KEYWORDS (the function that performs keyword matching against icon names—search for where CATEGORY_KEYWORDS is consumed) to check for whole-word boundaries or tokenized icon names before accepting "key" as a match, and ensure fallback behavior still works when no boundary match is found; keep the existing CATEGORY_KEYWORDS entries unchanged.
🧹 Nitpick comments (3)
tools/idea-plugin/src/main/resources/messages/Valkyrie.properties (1)
129-130: Nitpick: unit spacing inconsistency with the existingdpsuffix key.
{0}pxhas no space before the unit, while the pre-existingweb.import.font.customize.material.dp.suffixuses{0} dp(with a space). Neither is wrong on its own, but it's worth aligning them for consistency if this customization panel ever renders both units side-by-side.✏️ Option to align spacing
-web.import.font.customize.px.suffix={0}px +web.import.font.customize.px.suffix={0} px🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/resources/messages/Valkyrie.properties` around lines 129 - 130, The px suffix property is inconsistent with the dp suffix spacing; update the property value for web.import.font.customize.px.suffix from "{0}px" to "{0} px" so it matches the spacing style used by web.import.font.customize.material.dp.suffix — edit the messages entry for web.import.font.customize.px.suffix accordingly.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardIconViewModel.kt (1)
154-159: Potential race condition inupdateSuccesswith concurrent callers.
selectCategory(Line 123) andupdateSearchQuery(Line 133) both launch onDispatchers.Defaultand callupdateSuccess, which performs a non-atomic read-modify-write onstateRecord.value. If a user types a query while a category selection is still processing, one update could be lost.In practice this is unlikely since UI interactions are mostly sequential, but worth noting if concurrency increases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardIconViewModel.kt` around lines 154 - 159, The updateSuccess function does a non-atomic read-modify-write on stateRecord.value which can lose updates when multiple coroutines (e.g., selectCategory and updateSearchQuery) call it concurrently; change updateSuccess to perform an atomic update by using a compare-and-set loop (CAS) or the atomic update helper for your state type (e.g., MutableStateFlow.update or AtomicReference.compareAndSet): read current, compute new via transform(current), then attempt to atomically replace the value and retry on failure; keep the function name updateSuccess and ensure callers selectCategory and updateSearchQuery keep calling it unchanged.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kt (1)
25-33: Bootstrap icons lack tag metadata, reducing categorization and searchability
BootstrapRepository.loadIconList()returns only aMap<String, Int>(name→codepoint), while the semantically equivalentLucideRepository.loadIconList()returnsList<Pair<String, LucideIconMetadata>>with tags. This causesStandardIconinstances for Bootstrap to carry empty tags, which means:
- Filtering limitation: No tag-based search/filtering in the UI
- Categorization gap:
inferCategoryFromTags(name, emptyList())can only match on icon names; the tag-matching branch never fires. Icons with non-descriptive names fall back to"General"category instead of being properly categorizedFor consistency with Lucide and to improve UX, extend
BootstrapRepositoryto parse tags from the bootstrap-icons.json source and updateloadIconList()to return a richer type that includes them.♻️ Required changes
- Create a metadata DTO for Bootstrap icons (similar to
LucideIconMetadata)- Update
BootstrapRepository.loadIconList()to return structured metadata with tags- Update
BootstrapUseCase.loadConfig()to destructure tags and pass them to bothStandardIcon.tagsandinferCategoryFromTags()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kt` around lines 25 - 33, Bootstrap icons currently lose tag metadata because BootstrapRepository.loadIconList() returns Map<String,Int>; update it to parse bootstrap-icons.json into a metadata DTO (e.g., BootstrapIconMetadata with tags: List<String>) and change BootstrapRepository.loadIconList() to return List<Pair<String,BootstrapIconMetadata>> (matching LucideRepository.loadIconList() shape). Then update BootstrapUseCase.loadConfig() to destructure the returned pairs and populate StandardIcon(tags = metadata.tags) and call inferCategoryFromTags(name, metadata.tags) so tags are used for both filtering and category inference. Ensure DTO name and functions referenced are BootstrapIconMetadata, BootstrapRepository.loadIconList(), BootstrapUseCase.loadConfig(), StandardIcon and inferCategoryFromTags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-native-libs.yml:
- Around line 18-19: The workflow clones google/woff2 without pinning, causing
non-reproducible builds; update each job that runs the git clone (the steps in
the build-linux, build-windows, and build-macos jobs that run the command "git
clone --recursive --depth 1 https://github.com/google/woff2.git /tmp/woff2") to
pin to a fixed ref by using an environment variable (e.g. WOFF2_REF) and
appending "@<ref>" or switching to "git clone --branch $WOFF2_REF --depth 1
https://github.com/google/woff2.git /tmp/woff2" (choose a stable tag like v1.0.2
or a specific commit SHA) and ensure the same WOFF2_REF is used in all three
clone steps (repeat this change for the other two clone occurrences).
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/domain/model/font/FontSettings.kt`:
- Around line 18-19: The isModified getter in FontSettings currently ignores
changes to iconFontFamily; update the override of isModified to include a
comparison for iconFontFamily (e.g. add "|| iconFontFamily !=
<defaultIconFontFamily>" or compare against the initial/default value used by
FontSettings) so any change to iconFontFamily flips isModified to true; modify
the boolean expression in the isModified property in FontSettings.kt to include
that check.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kt`:
- Around line 21-26: The companion object constants UNPKG_BASE, FONT_URL and
JSON_URL in BootstrapRepository currently use the unstable "@latest" tag which
can cause font/codepoint mismatches; change the approach to pin a stable version
(e.g., replace "@latest" with a constant VERSION like "bootstrap-icons@1.11.3")
or implement logic in BootstrapRepository to resolve and cache the exact version
from the first fetched URL and reuse that resolved base for FONT_URL and
JSON_URL (update UNPKG_BASE, FONT_URL, JSON_URL and any consumers such as
ICONS_BASE_URL to reference the pinned VERSION or the resolved cached base so
both JSON and font use the same exact version).
---
Duplicate comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kt`:
- Around line 62-68: The parseCodepoints function correctly parses decimal
codepoints using toIntOrNull(), so no code change is needed; instead resolve the
duplicate review note by removing the duplicate comment marker or marking the
earlier comment as addressed in the PR discussion and confirm parseCodepoints
(function name parseCodepoints and usage of toIntOrNull on
value.jsonPrimitive.content) remains unchanged.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/CategoryInferrer.kt`:
- Around line 20-52: The CATEGORY_KEYWORDS list contains an entry
CategoryKeyword("key", "Security", priority = 1) which will match substrings
like "keyboard"; leave the keyword but make matching explicit to avoid
unintended hits by changing the matching logic to prefer whole-token matches or
boundary-aware checks: update the code that uses CATEGORY_KEYWORDS (the function
that performs keyword matching against icon names—search for where
CATEGORY_KEYWORDS is consumed) to check for whole-word boundaries or tokenized
icon names before accepting "key" as a match, and ensure fallback behavior still
works when no boundary match is found; keep the existing CATEGORY_KEYWORDS
entries unchanged.
---
Nitpick comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kt`:
- Around line 25-33: Bootstrap icons currently lose tag metadata because
BootstrapRepository.loadIconList() returns Map<String,Int>; update it to parse
bootstrap-icons.json into a metadata DTO (e.g., BootstrapIconMetadata with tags:
List<String>) and change BootstrapRepository.loadIconList() to return
List<Pair<String,BootstrapIconMetadata>> (matching
LucideRepository.loadIconList() shape). Then update
BootstrapUseCase.loadConfig() to destructure the returned pairs and populate
StandardIcon(tags = metadata.tags) and call inferCategoryFromTags(name,
metadata.tags) so tags are used for both filtering and category inference.
Ensure DTO name and functions referenced are BootstrapIconMetadata,
BootstrapRepository.loadIconList(), BootstrapUseCase.loadConfig(), StandardIcon
and inferCategoryFromTags.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardIconViewModel.kt`:
- Around line 154-159: The updateSuccess function does a non-atomic
read-modify-write on stateRecord.value which can lose updates when multiple
coroutines (e.g., selectCategory and updateSearchQuery) call it concurrently;
change updateSuccess to perform an atomic update by using a compare-and-set loop
(CAS) or the atomic update helper for your state type (e.g.,
MutableStateFlow.update or AtomicReference.compareAndSet): read current, compute
new via transform(current), then attempt to atomically replace the value and
retry on failure; keep the function name updateSuccess and ensure callers
selectCategory and updateSearchQuery keep calling it unchanged.
In `@tools/idea-plugin/src/main/resources/messages/Valkyrie.properties`:
- Around line 129-130: The px suffix property is inconsistent with the dp suffix
spacing; update the property value for web.import.font.customize.px.suffix from
"{0}px" to "{0} px" so it matches the spacing style used by
web.import.font.customize.material.dp.suffix — edit the messages entry for
web.import.font.customize.px.suffix accordingly.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tools/idea-plugin/src/main/resources/native/linux-x64/libwoff2decoder.sois excluded by!**/*.sotools/idea-plugin/src/main/resources/native/macos/libwoff2decoder.dylibis excluded by!**/*.dylibtools/idea-plugin/src/main/resources/native/windows-x64/woff2decoder.dllis excluded by!**/*.dll
📒 Files selected for processing (42)
.github/workflows/build-native-libs.ymlsdk/compose/icons/api/icons.apisdk/compose/icons/api/icons.klib.apisdk/compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/compose/icons/colored/BootstrapLogo.kttools/idea-plugin/CHANGELOG.mdtools/idea-plugin/native/woff2decoder.cpptools/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/model/IconSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/CustomizationToolbar.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.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/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/material/domain/model/font/FontSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/ui/FontCustomization.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardIconViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/BootstrapImportScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/di/BootstrapModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/CategoryInferrer.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/DisplayNameFormatter.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/StandardIconProvider.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/SvgSizeCustomizer.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/LucideImportScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/data/LucideIconMetadata.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/data/LucideRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/di/LucideModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/domain/LucideUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/InferredCategory.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/SizeSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/StandardIcon.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/StandardIconConfig.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/ui/StandardTopActions.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/util/StandardGridFiltering.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/Woff2Decoder.kttools/idea-plugin/src/main/resources/messages/Valkyrie.properties
💤 Files with no reviewable changes (5)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.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/lucide/domain/model/LucideSettings.kt
- 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/domain/model/LucideIcon.kt
✅ Files skipped from review due to trivial changes (2)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/SizeSettings.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/lucide/data/LucideRepository.kt
🚧 Files skipped from review as they are similar to previous changes (13)
- sdk/compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/compose/icons/colored/BootstrapLogo.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/Woff2Decoder.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/StandardIconProvider.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/standard/bootstrap/di/BootstrapModule.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt
- sdk/compose/icons/api/icons.klib.api
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/CustomizationToolbar.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/font/NativeLibraryLoader.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/StandardIconConfig.kt
- tools/idea-plugin/CHANGELOG.md
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/model/InferredCategory.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/domain/DisplayNameFormatter.kt
.../github/composegears/valkyrie/ui/screen/webimport/material/domain/model/font/FontSettings.kt
Show resolved
Hide resolved
...hub/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/data/BootstrapRepository.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt (3)
72-80: RedundantrememberUpdatedState—ObserveEventalready handles it internally.
ObserveEventwraps itsonEachlambda withrememberUpdatedStateinternally (seeEffect.kt). The outerrememberUpdatedState(onIconDownload)on line 72 and the extracurrentOnIconDownloadedindirection serve no additional purpose.♻️ Simplify by removing the redundant wrap
- val currentOnIconDownloaded by rememberUpdatedState(onIconDownload) - ObserveEvent(viewModel.events) { event -> when (event) { is StandardIconEvent.IconDownloaded -> { - currentOnIconDownloaded(event) + onIconDownload(event) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt` around lines 72 - 80, Remove the redundant rememberUpdatedState wrapper around onIconDownload: the ObserveEvent helper already uses rememberUpdatedState internally, so delete the line declaring currentOnIconDownloaded and call onIconDownload directly inside the ObserveEvent handler for StandardIconEvent.IconDownloaded; update the when branch in the ObserveEvent(viewModel.events) block to invoke onIconDownload(event) and remove any unused currentOnIconDownloaded symbol.
290-296: Replace unchecked castas StandardIconwith a safe cast.
IconItem<*>erases the icon's type toAny?, soitem.icon as StandardIconis an unchecked cast that generates a Kotlin compiler warning today and will silently throwClassCastExceptionif a non-StandardIconIconItemever appears ingridItems.♻️ Proposed fix using safe cast
- is IconItem<*> -> iconContent(item.icon as StandardIcon) + is IconItem<*> -> (item.icon as? StandardIcon)?.let { iconContent(it) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt` around lines 290 - 296, The lambda handling grid items uses an unchecked cast "item.icon as StandardIcon" which can throw; change it to a safe cast and guard before calling iconContent: cast item.icon to StandardIcon? (using a safe cast) and only call iconContent when the result is non-null (otherwise handle or skip), so update the branch for IconItem in the when expression to safely cast item.icon to StandardIcon and call iconContent with the safely-casted value, referencing IconItem, StandardIcon, iconContent, CategoryHeader and the gridItems source.
107-111: Prefermodifier.fillMaxSize()overModifier.fillMaxSize().then(modifier).The standard Compose convention is to chain internal modifiers onto the passed-in
modifier, so the caller's constraints take precedence. The current order discards any size/layout intentions from the caller.♻️ Proposed fix
- Column( - modifier = Modifier - .fillMaxSize() - .then(modifier), - ) { + Column(modifier = modifier.fillMaxSize()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt` around lines 107 - 111, The Column currently uses Modifier.fillMaxSize().then(modifier) which makes the local fillMaxSize override caller constraints; change the modifier expression to start from the incoming modifier and apply fillMaxSize onto it (i.e., use modifier.fillMaxSize()) in the Column call so the caller's modifiers take precedence; update the Column declaration that references modifier and fillMaxSize accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.kt`:
- Around line 35-42: The KDoc for IconSizeCustomization incorrectly calls
sizeLabel a "resource key" while the composable uses it as resolved text
(Text(text = sizeLabel) and previews pass a literal string); update the
declaration and KDoc for the IconSizeCustomization composable to state that
sizeLabel is already resolved display text (or alternatively, if you intended a
string resource id, change usages to call stringResource(sizeLabel) and make
sizeLabel an `@StringRes` Int); reference the IconSizeCustomization composable and
the sizeLabel parameter when making the change.
- Around line 52-79: The local slider state var size (created with remember {
mutableFloatStateOf(settings.size.toFloat()) }) can drift when the parent
updates settings.size; add a LaunchedEffect keyed on settings.size that updates
size = settings.size.toFloat() so the Slider and label stay in sync with
external changes while preserving in-flight slider precision; reference the size
var, the remember/mutableFloatStateOf initialization, and
LaunchedEffect(settings.size) when applying the change.
---
Nitpick comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt`:
- Around line 72-80: Remove the redundant rememberUpdatedState wrapper around
onIconDownload: the ObserveEvent helper already uses rememberUpdatedState
internally, so delete the line declaring currentOnIconDownloaded and call
onIconDownload directly inside the ObserveEvent handler for
StandardIconEvent.IconDownloaded; update the when branch in the
ObserveEvent(viewModel.events) block to invoke onIconDownload(event) and remove
any unused currentOnIconDownloaded symbol.
- Around line 290-296: The lambda handling grid items uses an unchecked cast
"item.icon as StandardIcon" which can throw; change it to a safe cast and guard
before calling iconContent: cast item.icon to StandardIcon? (using a safe cast)
and only call iconContent when the result is non-null (otherwise handle or
skip), so update the branch for IconItem in the when expression to safely cast
item.icon to StandardIcon and call iconContent with the safely-casted value,
referencing IconItem, StandardIcon, iconContent, CategoryHeader and the
gridItems source.
- Around line 107-111: The Column currently uses
Modifier.fillMaxSize().then(modifier) which makes the local fillMaxSize override
caller constraints; change the modifier expression to start from the incoming
modifier and apply fillMaxSize onto it (i.e., use modifier.fillMaxSize()) in the
Column call so the caller's modifiers take precedence; update the Column
declaration that references modifier and fillMaxSize accordingly.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
sdk/compose/icons/api/icons.apisdk/compose/icons/api/icons.klib.apitools/idea-plugin/CHANGELOG.mdtools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt
...otlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.kt
Show resolved
Hide resolved
...otlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.kt
Show resolved
Hide resolved
egorikftp
force-pushed
the
feature/bootstrap-icons
branch
from
6957fe0 to
5bafdc0
Compare
There was a problem hiding this comment.
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/standard/StandardImportScreenUI.kt (1)
285-296:⚠️ Potential issue | 🟡 MinorUnsafe unchecked cast risks a
ClassCastException
item.icon as StandardIcononIconItem<*>is an unchecked cast — JVM type erasure means the type argument is invisible at runtime. If the grid is ever accidentally fed anIconItemwhose payload is not aStandardIcon, this will crash with no compile-time warning.Prefer the safe-cast + early-return form:
🛡️ Proposed fix
- is IconItem<*> -> iconContent(item.icon as StandardIcon) + is IconItem<*> -> { + val icon = item.icon as? StandardIcon ?: return@items + iconContent(icon) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt` around lines 285 - 296, The branch handling IconItem in StandardImportScreenUI.kt currently does an unsafe cast (item.icon as StandardIcon) which can throw ClassCastException; replace it with a safe cast and early-return/fallback: inside the IconItem branch of the grid content lambda, use a safe cast (item.icon as? StandardIcon) and if null either skip rendering or render a fallback (e.g., placeholder) instead of casting; update the call site to pass the safely-cast value to iconContent (refer to IconItem, StandardIcon, iconContent, and CategoryHeader to locate the code).
♻️ Duplicate comments (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.kt (1)
52-79:⚠️ Potential issue | 🟡 MinorMissing
LaunchedEffectto re-sync slider whensettings.sizechanges externally.The
onResetlambda explicitly resets the localsizevariable (line 58), so the reset path is safe. However,sizeis initialised once viarememberand will silently drift if the parent changessettings.sizethrough any path other thanonReset(e.g., ViewModel state restoration, external preset selection). Adding aLaunchedEffectkeyed onsettings.sizeguards against this without affecting in-flight slider precision.🔧 Proposed fix
+import androidx.compose.runtime.LaunchedEffect ... var size by remember { mutableFloatStateOf(settings.size.toFloat()) } + LaunchedEffect(settings.size) { + if (settings.size != size.roundToInt()) { + size = settings.size.toFloat() + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.kt` around lines 52 - 79, The local mutable "size" (created with remember { mutableFloatStateOf(settings.size.toFloat()) }) can drift if settings.size changes externally; add a LaunchedEffect keyed on settings.size that updates the local size (e.g., set size = settings.size.toFloat()) so the Slider and UI re-sync when the parent/VM updates settings, while leaving onReset and onSettingsChange behavior intact; place this LaunchedEffect near the size declaration in IconSizeCustomization.kt to ensure correct scoping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt`:
- Around line 285-296: The branch handling IconItem in StandardImportScreenUI.kt
currently does an unsafe cast (item.icon as StandardIcon) which can throw
ClassCastException; replace it with a safe cast and early-return/fallback:
inside the IconItem branch of the grid content lambda, use a safe cast
(item.icon as? StandardIcon) and if null either skip rendering or render a
fallback (e.g., placeholder) instead of casting; update the call site to pass
the safely-cast value to iconContent (refer to IconItem, StandardIcon,
iconContent, and CategoryHeader to locate the code).
---
Duplicate comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.kt`:
- Around line 52-79: The local mutable "size" (created with remember {
mutableFloatStateOf(settings.size.toFloat()) }) can drift if settings.size
changes externally; add a LaunchedEffect keyed on settings.size that updates the
local size (e.g., set size = settings.size.toFloat()) so the Slider and UI
re-sync when the parent/VM updates settings, while leaving onReset and
onSettingsChange behavior intact; place this LaunchedEffect near the size
declaration in IconSizeCustomization.kt to ensure correct scoping.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
sdk/compose/icons/api/icons.apisdk/compose/icons/api/icons.klib.apitools/idea-plugin/CHANGELOG.mdtools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/IconSizeCustomization.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/StandardImportScreenUI.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/idea-plugin/CHANGELOG.md
- sdk/compose/icons/api/icons.klib.api
egorikftp
approved these changes
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.