Open
Conversation
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Font Awesome support and related infrastructure: updates the version catalog to include 🚥 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
🧹 Nitpick comments (2)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardIconViewModel.kt (1)
215-231: Per-style prefetch re-loads shared-font providers.This loop keys the cache by style id, but
BoxIconsUseCase.loadFontBytes()intools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/boxicons/domain/BoxIconsUseCase.ktignores the style and always fetches the same font. Opening BoxIcons will now schedule the same font load for regular, solid, and logos, then keep three copies of identical bytes in memory. Consider letting providers expose a shared font-cache key or opt out of prefetch when styles reuse one font.🤖 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/common/StandardIconViewModel.kt` around lines 215 - 231, prefetchStyleFonts is caching font bytes by style.id but provider.loadFontBytes (e.g., BoxIconsUseCase.loadFontBytes) returns the same shared font for multiple styles, causing duplicate cached byte arrays; change the prefetch logic to use a provider-level font cache key (e.g., provider.fontCacheKey or provider.getFontKey(style)) or skip prefetch for styles that map to the same key: compute a unique fontKey for each style (call provider method or add one to the provider interface), check fontCache[fontKey] instead of fontCache[style.id], and only call provider.loadFontBytes(style) when that fontKey is not already cached so identical fonts are fetched/stored once.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeRepository.kt (1)
64-72: WOFF2 decoding failure path propagates an opaque error.When
Woff2Decoder.decodeBytesreturnsnull, the genericerror()message provides no context about which font failed. Consider including the URL or style ID in the error message to aid debugging.🔧 Proposed improvement
private suspend fun loadAndDecodeWoff2(url: String): ByteArray { return withContext(Dispatchers.IO) { val woff2Bytes = httpClient.get(url).bodyAsChannel().toByteArray() withContext(Dispatchers.Default) { - Woff2Decoder.decodeBytes(woff2Bytes) ?: error("Failed to decode WOFF2 font") + Woff2Decoder.decodeBytes(woff2Bytes) + ?: error("Failed to decode WOFF2 font from: $url") } } }🤖 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/fontawesome/data/FontAwesomeRepository.kt` around lines 64 - 72, In loadAndDecodeWoff2, when Woff2Decoder.decodeBytes(woff2Bytes) returns null replace the opaque error() call with an error message that includes identifying context (at minimum the url, and optionally a style or font id if available) so failures show which font failed to decode; update the thrown exception message to include url and the fact decoding returned null and keep this change scoped to loadAndDecodeWoff2 and the decodeBytes result handling.
🤖 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/common/StandardIconViewModel.kt`:
- Around line 48-67: In the StandardState.Success branch adjust handling of
initialState.fontByteArray so we only reuse/cache it when the saved style
matches the normalizedSelectedStyle: compute normalizedSelectedStyle as you
already do, then if initialState.fontByteArray != null check that
initialState.selectedStyle?.id == normalizedSelectedStyle?.id before storing
into fontCache (fontCache[styleKey] = it); otherwise clear/ignore
initialState.fontByteArray and call downloadFont(normalizedSelectedStyle) to
force re-download; update the logic around downloadFont(normalizedSelectedStyle)
and fontByteArray to reflect this conditional.
- Around line 170-187: selectStyle currently reads the shared mutable map
fontCache inside viewModelScope.launch(Dispatchers.Default), causing potential
data races because other reads/writes (around updateSuccess and downloadFont)
happen on the main dispatcher; fix by either (A) switching the cache read to run
on Dispatchers.Main (e.g., perform fontCache[style.id] lookup inside a
withContext(Dispatchers.Main) block before calling updateSuccess and
downloadFont) or (B) make fontCache thread-safe (replace mutableMapOf with a
synchronized map or guard all accesses to fontCache with a Mutex) so that the
lookup in selectStyle and writes in downloadFont are safely synchronized.
---
Nitpick comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardIconViewModel.kt`:
- Around line 215-231: prefetchStyleFonts is caching font bytes by style.id but
provider.loadFontBytes (e.g., BoxIconsUseCase.loadFontBytes) returns the same
shared font for multiple styles, causing duplicate cached byte arrays; change
the prefetch logic to use a provider-level font cache key (e.g.,
provider.fontCacheKey or provider.getFontKey(style)) or skip prefetch for styles
that map to the same key: compute a unique fontKey for each style (call provider
method or add one to the provider interface), check fontCache[fontKey] instead
of fontCache[style.id], and only call provider.loadFontBytes(style) when that
fontKey is not already cached so identical fonts are fetched/stored once.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeRepository.kt`:
- Around line 64-72: In loadAndDecodeWoff2, when
Woff2Decoder.decodeBytes(woff2Bytes) returns null replace the opaque error()
call with an error message that includes identifying context (at minimum the
url, and optionally a style or font id if available) so failures show which font
failed to decode; update the thrown exception message to include url and the
fact decoding returned null and keep this change scoped to loadAndDecodeWoff2
and the decodeBytes result handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d8c4a5b6-6708-41d8-9690-d73cae0cbf2e
📒 Files selected for processing (30)
gradle/libs.versions.tomlsdk/compose/icons/api/icons.apisdk/compose/icons/api/icons.klib.apisdk/compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/compose/icons/colored/FontAwesomeLogo.kttools/idea-plugin/CHANGELOG.mdtools/idea-plugin/build.gradle.ktstools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/service/PersistentSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/settings/InMemorySettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportFlow.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/boxicons/domain/BoxIconsUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardIconViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardImportScreenUI.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/domain/StandardIconProvider.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/model/StandardIconConfig.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/util/StandardGridFiltering.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/FontAwesomeImportScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeCategoriesYamlParser.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconMetadata.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconsYamlParser.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeYamlUtil.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/di/FontAwesomeModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/domain/FontAwesomeUseCase.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/remix/domain/RemixUseCase.kttools/idea-plugin/src/main/resources/messages/Valkyrie.propertiestools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeCategoriesYamlParserTest.kttools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconsYamlParserTest.kt
💤 Files with no reviewable changes (1)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/model/StandardIconConfig.kt
...io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardIconViewModel.kt
Show resolved
Hide resolved
...io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardIconViewModel.kt
Show resolved
Hide resolved
egorikftp
reviewed
Mar 6, 2026
egorikftp
reviewed
Mar 6, 2026
| companion object { | ||
| private const val CDN_BASE = "https://cdn.jsdelivr.net/npm/@fortawesome/fontawesome-free@latest" | ||
| private const val ICONS_YML_URL = "$CDN_BASE/metadata/icons.yml" | ||
| private const val CATEGORIES_YML_URL = "$CDN_BASE/metadata/categories.yml" |
Member
There was a problem hiding this comment.
They have css files splitted by category
https://www.npmjs.com/package/@fortawesome/fontawesome-free?activeTab=code
Collaborator
Author
There was a problem hiding this comment.
I will blame it on working late but I thought there was no solid.css files there.
At the moment I'm using:
- icons.yml for codepoints, search terms, and styles
- categories.yml for category grouping
So I think the main question is which direction we want here:
- Use CSS for codepoint/style extraction to stay consistent with the other providers, but keep YAML for categories.
- Use CSS only, and fall back to inferred names/categories/tags where needed.
I lean toward keeping the YAML categories personally but happy to remove it completely too, what do you think?
Member
There was a problem hiding this comment.
Well, I checked again and looks like css files not contains all necessary info
Collaborator
Author
There was a problem hiding this comment.
Think we can leave as is then?
Member
There was a problem hiding this comment.
I think yes. One concern is that we are using an archived library for YAML.
Maybe we can migrate to the library mentioned in the official Kotlin serialization repository: https://github.com/him188/yamlkt
egorikftp
reviewed
Mar 6, 2026
...composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeRepository.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeRepository.kt (1)
20-20: Consider pinning a specific Font Awesome version.Using
@latestmakes builds non-reproducible and could break unexpectedly if Font Awesome changes their CDN structure or YAML schema. If stability is a concern, consider pinning a specific version (e.g.,@6.5.1) and updating it periodically.🤖 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/fontawesome/data/FontAwesomeRepository.kt` at line 20, The CDN_BASE constant in FontAwesomeRepository currently uses the non-reproducible "@latest" tag; change CDN_BASE to reference a specific Font Awesome release (e.g., "@6.5.1") to pin the dependency and ensure reproducible builds, or make the version configurable via a VERSION constant or constructor parameter so updates are deliberate and tracked (update the CDN_BASE declaration in FontAwesomeRepository 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/standard/fontawesome/data/FontAwesomeRepository.kt`:
- Around line 51-54: loadFontBytes currently silently falls back to
SOLID_STYLE_ID when an unknown styleId is passed; update loadFontBytes to handle
invalid style IDs explicitly by checking fontsByStyleId.containsKey(styleId) and
either (A) throw an IllegalArgumentException (or custom exception) referencing
the invalid styleId, or (B) log a warning via the existing logger that styleId
is unknown and you are falling back to SOLID_STYLE_ID before calling the loader;
make the change inside loadFontBytes and keep the existing fallback behavior
only if you choose the logging option.
---
Nitpick comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeRepository.kt`:
- Line 20: The CDN_BASE constant in FontAwesomeRepository currently uses the
non-reproducible "@latest" tag; change CDN_BASE to reference a specific Font
Awesome release (e.g., "@6.5.1") to pin the dependency and ensure reproducible
builds, or make the version configurable via a VERSION constant or constructor
parameter so updates are deliberate and tracked (update the CDN_BASE declaration
in FontAwesomeRepository accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d56aae9e-9e14-46fe-88a8-aabcbad85ace
📒 Files selected for processing (2)
tools/idea-plugin/CHANGELOG.mdtools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeRepository.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/idea-plugin/CHANGELOG.md
...composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeRepository.kt
Show resolved
Hide resolved
t-regbs
force-pushed
the
feature/font-awesome-icons
branch
from
5ae592f to
bd5649c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardImportScreenUI.kt (1)
240-243: Consider moving alias resolution into the provider contract.
"$fontAlias-${style.id}"adds a second implicit style-to-font mapping rule in the UI. A smallresolveFontAlias(style)helper onStandardIconProviderwould keep that contract in one place and avoid future drift.🤖 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/common/StandardImportScreenUI.kt` around lines 240 - 243, The UI is constructing a style-aware alias inline via "$fontAlias-${style.id}" which duplicates mapping logic; add a resolveFontAlias(style) (or resolveFontAlias(fontAlias, style)) helper to the StandardIconProvider contract and replace the inline logic in StandardImportScreenUI where styleAwareAlias is computed (using state.selectedStyle and fontAlias) with a call to that provider method so alias resolution lives in the provider API and not in the UI.
🤖 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/fontawesome/data/FontAwesomeRepository.kt`:
- Around line 20-28: The CDN_BASE constant currently uses "@latest", which risks
inconsistent upstream changes across ICONS_YML_URL, CATEGORIES_YML_URL,
FONT_SOLID, FONT_REGULAR and FONT_BRANDS; change CDN_BASE to reference a
concrete, tested Font Awesome release (e.g., replace "@latest" with a pinned
version string) so all derived constants (ICONS_YML_URL, CATEGORIES_YML_URL,
FONT_SOLID, FONT_REGULAR, FONT_BRANDS) resolve to the same version, and consider
introducing a single VERSION constant to make future updates intentional and
visible.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/domain/FontAwesomeUseCase.kt`:
- Around line 34-36: The resolveFontWeight implementation is inconsistent with
loadFontBytes: it maps a null IconStyle to W400 while loadFontBytes treats null
as the solid style; update resolveFontWeight (function resolveFontWeight in
FontAwesomeUseCase) so that when style is null it returns FontWeight.W900 (same
as when style?.id == SOLID_STYLE_ID) instead of W400, ensuring glyph selection
matches loadFontBytes' null-as-solid behavior.
---
Nitpick comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardImportScreenUI.kt`:
- Around line 240-243: The UI is constructing a style-aware alias inline via
"$fontAlias-${style.id}" which duplicates mapping logic; add a
resolveFontAlias(style) (or resolveFontAlias(fontAlias, style)) helper to the
StandardIconProvider contract and replace the inline logic in
StandardImportScreenUI where styleAwareAlias is computed (using
state.selectedStyle and fontAlias) with a call to that provider method so alias
resolution lives in the provider API and not in the UI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8fcecf95-26f3-4a46-b9f9-d7f6ff8f850e
📒 Files selected for processing (30)
gradle/libs.versions.tomlsdk/compose/icons/api/icons.apisdk/compose/icons/api/icons.klib.apisdk/compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/compose/icons/colored/FontAwesomeLogo.kttools/idea-plugin/CHANGELOG.mdtools/idea-plugin/build.gradle.ktstools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/service/PersistentSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/settings/InMemorySettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportFlow.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/boxicons/domain/BoxIconsUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardIconViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardImportScreenUI.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/domain/StandardIconProvider.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/model/StandardIconConfig.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/util/StandardGridFiltering.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/FontAwesomeImportScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeCategoriesYamlParser.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconMetadata.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconsYamlParser.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeYamlUtil.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/di/FontAwesomeModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/domain/FontAwesomeUseCase.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/remix/domain/RemixUseCase.kttools/idea-plugin/src/main/resources/messages/Valkyrie.propertiestools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeCategoriesYamlParserTest.kttools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconsYamlParserTest.kt
💤 Files with no reviewable changes (1)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/model/StandardIconConfig.kt
🚧 Files skipped from review as they are similar to previous changes (8)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/service/PersistentSettings.kt
- tools/idea-plugin/src/main/resources/messages/Valkyrie.properties
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/remix/domain/RemixUseCase.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/domain/StandardIconProvider.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/standard/fontawesome/data/FontAwesomeIconMetadata.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/settings/InMemorySettings.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeCategoriesYamlParser.kt
...composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeRepository.kt
Show resolved
Hide resolved
.../composegears/valkyrie/ui/screen/webimport/standard/fontawesome/domain/FontAwesomeUseCase.kt
Show resolved
Hide resolved
t-regbs
force-pushed
the
feature/font-awesome-icons
branch
from
bd5649c to
ce58438
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tools/idea-plugin/CHANGELOG.md (1)
7-7: LGTM! Changelog entry is well-formatted and correctly placed.The entry follows the established pattern for web import provider additions and is properly placed in the "Unreleased > Added" section. The formatting (with backticks around
Font Awesome) is consistent with other provider entries.Optional: Consider grouping related entries
For improved readability, you might consider placing this entry near the other icon provider entries (Bootstrap on line 22, Remix on line 25, Box on line 26). This would group related features together, though the current placement is perfectly acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/CHANGELOG.md` at line 7, Move the changelog entry "- [Web Import] Add `Font Awesome` icons provider" so it sits alongside the other icon provider entries (e.g., the existing Bootstrap, Remix, Box entries) to group related items for readability; edit tools/idea-plugin/CHANGELOG.md and reposition the line containing the `Font Awesome` entry near the other icon provider lines while preserving formatting and the "Unreleased > Added" section.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/domain/FontAwesomeUseCase.kt (1)
83-92: Centralize the Font Awesome style IDs.
solid,regular, andbrandsare duplicated here and inFontAwesomeRepository. That creates an easy desync point the next time a style is added or renamed:loadConfig()can expose a style that the repository cannot load/download. Hoisting them into one shared object would keep both sides locked together.🤖 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/fontawesome/domain/FontAwesomeUseCase.kt` around lines 83 - 92, The constants SOLID_STYLE_ID, REGULAR_STYLE_ID, BRANDS_STYLE_ID and the STYLE_BY_ID map in FontAwesomeUseCase should be centralized so FontAwesomeRepository and loadConfig() use a single source of truth; create a shared object (e.g., FontAwesomeStyles) that exposes the three IDs and the STYLE_BY_ID (or equivalent list/map) and replace the local constants and map in FontAwesomeUseCase and any references in FontAwesomeRepository/loadConfig() to use that shared object’s symbols instead.
🤖 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/common/util/StandardGridFiltering.kt`:
- Around line 25-27: The category filtering change switched from name-based to
id-based matching and breaks compatibility with the GridFiltering contract;
update the logic in StandardGridFiltering (the categoryFiltered calculation) to
preserve backward compatibility by either matching on both InferredCategory.id
and InferredCategory.name (e.g., accept items whose key.id == category.id OR
key.name == category.name) or by falling back to name-based matching when an
id-based filter yields no results; ensure InferredCategory.All still returns
gridItems and reference InferredCategory.All, gridItems, and the GridFiltering
contract when locating the code to modify.
---
Nitpick comments:
In `@tools/idea-plugin/CHANGELOG.md`:
- Line 7: Move the changelog entry "- [Web Import] Add `Font Awesome` icons
provider" so it sits alongside the other icon provider entries (e.g., the
existing Bootstrap, Remix, Box entries) to group related items for readability;
edit tools/idea-plugin/CHANGELOG.md and reposition the line containing the `Font
Awesome` entry near the other icon provider lines while preserving formatting
and the "Unreleased > Added" section.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/domain/FontAwesomeUseCase.kt`:
- Around line 83-92: The constants SOLID_STYLE_ID, REGULAR_STYLE_ID,
BRANDS_STYLE_ID and the STYLE_BY_ID map in FontAwesomeUseCase should be
centralized so FontAwesomeRepository and loadConfig() use a single source of
truth; create a shared object (e.g., FontAwesomeStyles) that exposes the three
IDs and the STYLE_BY_ID (or equivalent list/map) and replace the local constants
and map in FontAwesomeUseCase and any references in
FontAwesomeRepository/loadConfig() to use that shared object’s symbols instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb014fa4-304a-4801-b031-27749796f106
📒 Files selected for processing (30)
gradle/libs.versions.tomlsdk/compose/icons/api/icons.apisdk/compose/icons/api/icons.klib.apisdk/compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/compose/icons/colored/FontAwesomeLogo.kttools/idea-plugin/CHANGELOG.mdtools/idea-plugin/build.gradle.ktstools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/service/PersistentSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/settings/InMemorySettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportFlow.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/boxicons/domain/BoxIconsUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardIconViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardImportScreenUI.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/domain/StandardIconProvider.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/model/StandardIconConfig.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/util/StandardGridFiltering.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/FontAwesomeImportScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeCategoriesYamlParser.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconMetadata.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconsYamlParser.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeYamlUtil.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/di/FontAwesomeModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/domain/FontAwesomeUseCase.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/remix/domain/RemixUseCase.kttools/idea-plugin/src/main/resources/messages/Valkyrie.propertiestools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeCategoriesYamlParserTest.kttools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconsYamlParserTest.kt
💤 Files with no reviewable changes (1)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/model/StandardIconConfig.kt
🚧 Files skipped from review as they are similar to previous changes (15)
- sdk/compose/icons/api/icons.api
- gradle/libs.versions.toml
- sdk/compose/icons/api/icons.klib.api
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportFlow.kt
- tools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconsYamlParserTest.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/bootstrap/domain/BootstrapUseCase.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/domain/StandardIconProvider.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeYamlUtil.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconMetadata.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeIconsYamlParser.kt
- tools/idea-plugin/src/main/resources/messages/Valkyrie.properties
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/settings/InMemorySettings.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/di/FontAwesomeModule.kt
- tools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/fontawesome/data/FontAwesomeCategoriesYamlParserTest.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/StandardImportScreenUI.kt
Comment on lines
+25
to
+27
| val categoryFiltered = when { | ||
| category.id == InferredCategory.All.id -> gridItems | ||
| else -> gridItems.filterKeys { it.id == category.id } |
There was a problem hiding this comment.
Keep category matching backward-compatible.
Line 27 changes this helper from name-based matching to id-based matching, but the shared grid filtering contract in tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/util/GridFiltering.kt:34-41 still matches by name. Since InferredCategory.id and InferredCategory.name are intentionally not always equal (tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/model/InferredCategory.kt:1-16), this can make existing standard providers stop returning icons for categories whose slug and display name differ. Either migrate the contract end-to-end in this PR or keep a compatibility fallback here.
Suggested compatibility-safe change
val categoryFiltered = when {
category.id == InferredCategory.All.id -> gridItems
- else -> gridItems.filterKeys { it.id == category.id }
+ else -> gridItems.filterKeys { key ->
+ key.id == category.id || key.name == category.name
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val categoryFiltered = when { | |
| category.id == InferredCategory.All.id -> gridItems | |
| else -> gridItems.filterKeys { it.id == category.id } | |
| val categoryFiltered = when { | |
| category.id == InferredCategory.All.id -> gridItems | |
| else -> gridItems.filterKeys { key -> | |
| key.id == category.id || key.name == category.name | |
| } |
🤖 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/common/util/StandardGridFiltering.kt`
around lines 25 - 27, The category filtering change switched from name-based to
id-based matching and breaks compatibility with the GridFiltering contract;
update the logic in StandardGridFiltering (the categoryFiltered calculation) to
preserve backward compatibility by either matching on both InferredCategory.id
and InferredCategory.name (e.g., accept items whose key.id == category.id OR
key.name == category.name) or by falling back to name-based matching when an
id-based filter yields no results; ensure InferredCategory.All still returns
gridItems and reference InferredCategory.All, gridItems, and the GridFiltering
contract when locating the code to modify.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Screen.Recording.2026-03-06.at.00.16.36.mov
📝 Changelog
If this PR introduces user-facing changes, please update the relevant Unreleased section in changelogs: