Add case-insensitive duplicate detection for icon names#895
Merged
Conversation
egorikftp
force-pushed
the
bug/case-insensitive
branch
from
d14a118 to
5356322
Compare
WalkthroughAdds validations across CLI, Gradle plugin, and IDEA plugin to detect exact duplicate icon names and case-insensitive duplicates. Integrates duplicate checks before generation (CLI/Gradle) with nested-pack and useFlatPackage-aware grouping. Extends IDEA plugin validation API (adds ValidationError.HasCaseInsensitiveDuplicates and updates List.checkImportIssues(useFlatPackage: Boolean)). Adds unit tests covering exact and case-insensitive collision scenarios, a new IconNameFormatterTest case, small UI synchronization in ConfirmTextField, and updates changelogs. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/textfield/ConfirmTextField.kt (1)
46-50: Consider guarding against overwriting in-progress user edits while focusedThe
LaunchedEffectcorrectly syncs external prop changes (e.g., after collision-based name normalization) into the internal state. The equality guard prevents spurious updates.One edge case: if the parent updates
textwhile the user is actively editing (field is focused), this will silently overwrite their in-progress input. Adding a!focusedguard avoids that disruption while still propagating updates when the field is idle.♻️ Proposed optional guard
- // Update text field state when external text changes - LaunchedEffect(text) { - if (state.text.toString() != text) { - state.setTextAndPlaceCursorAtEnd(text) - } - } + // Update text field state when external text changes (only when not actively editing) + LaunchedEffect(text) { + if (!focused && state.text.toString() != text) { + state.setTextAndPlaceCursorAtEnd(text) + } + }🤖 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/jewel/textfield/ConfirmTextField.kt` around lines 46 - 50, The LaunchedEffect(text) block can overwrite an actively edited value; modify the condition to only apply external text when the text differs AND the field is not focused (i.e., add a !focused check) so updates propagate when idle but don't clobber in-progress edits; update the LaunchedEffect(text) condition that calls state.setTextAndPlaceCursorAtEnd(text) to include the focused guard.components/parser/unified/src/commonTest/kotlin/io/github/composegears/valkyrie/parser/unified/util/IconNameFormatterTest.kt (1)
53-65: Consider renaming for accuracy.The test verifies formatter output, not collision detection logic — that's exercised in
IconValidationTestandFailedConfigurationTest. A name likecheck formatting that produces case-insensitive collisionwould be more precise, but this is purely a nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/parser/unified/src/commonTest/kotlin/io/github/composegears/valkyrie/parser/unified/util/IconNameFormatterTest.kt` around lines 53 - 65, Rename the test function currently named `check case-insensitive collision detection` to a clearer name that reflects it asserts formatter output, e.g. `check formatting that produces case-insensitive collision`, and update any references; this is the test that calls `IconNameFormatter.format(fileName)` with `IconTest` cases (the two inputs "test-icon.svg" and "testicon.svg") and should not be confused with collision logic exercised elsewhere (`IconValidationTest`, `FailedConfigurationTest`).tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt (2)
422-455: Duplicate logic: inline nested-pack checks replicatecheckDuplicatesInIconNames.The exact and case-insensitive detection code inside
iconsByPack.forEach(lines 426–455) is a verbatim copy ofcheckDuplicatesInIconNames. The only difference is how thecontextstring is formatted. Delegate to the existing helper to remove the duplication.♻️ Proposed refactor
iconsByPack.forEach { (packIdentifier, filesInPack) -> val names = filesInPack.map { IconNameFormatter.format(name = it.name) } - - // Check exact duplicates within this pack/group - val exactDuplicates = names - .groupBy { it } - .filter { it.value.size > 1 } - .keys - .toList() - - if (exactDuplicates.isNotEmpty()) { - throw GradleException( - "Found duplicate icon names in \"$packIdentifier\": ${exactDuplicates.joinToString(", ")}. " + - "Each icon must have a unique name. " + - "Please rename the source files to avoid duplicates.", - ) - } - - // Check case-insensitive duplicates within this pack/group - val caseInsensitiveDuplicates = names - .groupBy { it.lowercase() } - .filter { it.value.size > 1 && it.value.distinct().size > 1 } - .values - .flatten() - .distinct() - - if (caseInsensitiveDuplicates.isNotEmpty()) { - throw GradleException( - "Found icon names that would collide on case-insensitive file systems (macOS/Windows) in \"$packIdentifier\": " + - "${caseInsensitiveDuplicates.joinToString(", ")}. " + - "These icons would overwrite each other during generation. " + - "Please rename the source files to avoid case-insensitive duplicates.", - ) - } + checkDuplicatesInIconNames(names, "\"$packIdentifier\"") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt` around lines 422 - 455, The inline duplicate detection inside iconsByPack.forEach duplicates the logic in checkDuplicatesInIconNames; replace the exact- and case-insensitive checks with a call to checkDuplicatesInIconNames by passing the computed names list and a context string built from packIdentifier (e.g., "pack \"$packIdentifier\"" or the same format used elsewhere for consistency). Locate the block that computes names (IconNameFormatter.format) and remove the two duplicate-check blocks, invoking checkDuplicatesInIconNames(names, context) instead so all duplicate handling is centralized in that helper.
399-399:iconNamesparameter is unused in the nested-packs path.When
nestedPacks.isNotEmpty(), icon names are recomputed insidevalidateDuplicatesviaIconNameFormatter.formatinstead of using the pre-computediconNames. The parameter is only consumed by the twoelsebranches. This leaks the caller's internal structure through the signature and causes double-invocation ofIconNameFormatter.formatfor nested-pack scenarios.Consider replacing the
iconNames: List<String>parameter with aMap<File, String>(pre-computed once at the call site and looked up per file inside the function), or simply removing the parameter and always deriving names fromfilesinside the function, which would be consistent across all branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt` at line 399, The validateDuplicates function currently takes iconNames but recomputes names for the nested-packs branch using IconNameFormatter.format, causing double work and leaking caller structure; change the signature of validateDuplicates to accept a Map<File, String> (precompute icon names once at the call site using IconNameFormatter.format for each File) or drop the parameter and always derive names from files inside validateDuplicates, then update all call sites to either pass the Map<File,String> or let the function compute names; inside validateDuplicates (and the nestedPacks branch) use the map lookup (or the single consistent derivation path) instead of calling IconNameFormatter.format again to eliminate the duplicate computation and remove the unused iconNames usage.tools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidationTest.kt (1)
167-175: Minor test redundancy with the pre-existingnested, allow duplicates for different currentNestedPacktest.Both this test and the one at line 135 assert that same-named icons in different nested packs are allowed when not using flat package — the only difference is that this one passes
useFlatPackage = falseexplicitly. If the intent is to document the explicit API shape, that's reasonable, but you may want to drop the older test to avoid maintaining two assertions for the same invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidationTest.kt` around lines 167 - 175, The new test `nested without flatPackage, allow exact duplicates in different nested packs` duplicates the existing `nested, allow duplicates for different currentNestedPack` test; either remove the older `nested, allow duplicates for different currentNestedPack` test or instead change one of them to explicitly document the API difference (e.g., keep the new test to assert the explicit parameter `useFlatPackage = false` and delete or rename the older test) so you don't maintain two assertions for the same invariant; update/remove the redundant test that uses validIcon, nestedPack, and checkImportIssues to keep a single clear test.tools/cli/src/main/kotlin/io/github/composegears/valkyrie/cli/command/SvgXmlToImageVectorCommand.kt (1)
221-251: Duplicate detection logic is correct; minor note on output ordering.The two-stage validation (exact duplicates first, then case-insensitive) is well-structured. The case-insensitive check correctly excludes already-caught exact duplicates via the
distinct().size > 1guard.One subtle point: both
exactDuplicates(viagroupBy {}.keys) andcaseInsensitiveDuplicates(via.flatten().distinct()) produce lists whose iteration order depends on the insertion order oficonNames, which in turn depends on the filesystem-dependent order oflistDirectoryEntries. The error message content is deterministic in set membership but non-deterministic in ordering. This is fine for user-facing output but makes exact string-matching in tests fragile (see test file comment).Consider sorting the duplicate names in the error messages for deterministic output
val exactDuplicates = iconNames .groupBy { it } .filter { it.value.size > 1 } .keys - .toList() + .sorted() ... val caseInsensitiveDuplicates = iconNames .groupBy { it.lowercase() } .filter { it.value.size > 1 && it.value.distinct().size > 1 } .values .flatten() .distinct() + .sorted()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/src/main/kotlin/io/github/composegears/valkyrie/cli/command/SvgXmlToImageVectorCommand.kt` around lines 221 - 251, The duplicate-name error messages are non-deterministic because exactDuplicates and caseInsensitiveDuplicates derive order from iconNames; modify the construction or the join step to sort the names before reporting so output is deterministic for tests—specifically, when building exactDuplicates and caseInsensitiveDuplicates (symbols: exactDuplicates, caseInsensitiveDuplicates, iconNames) or immediately before calling outputError, call a deterministic sort (e.g., sort() on the list) and then joinToString to produce the error text for both outputError calls.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt (1)
274-332: Two-stage dedup is solid; edge case with cascading name collisions.The sequential exact→case-insensitive resolution approach is well-structured. One edge case to be aware of: stage 1 renaming (e.g.,
"Test"→"Test1") could introduce a new collision with a pre-existing icon named"Test1". This wouldn't be caught by stage 2 since it's an exact (not case-insensitive) duplicate.This is acceptable for an auto-resolve UI action since:
checkImportIssuesis called on the result (line 341), so the new collision would surface as a validation error.- The user can manually rename afterward.
If you ever want to harden this, a loop-until-stable approach would handle cascading collisions:
// Pseudocode: keep resolving until no new issues arise var current = processedIcons do { val resolved = resolveOnce(current) val stable = resolved == current current = resolved } while (!stable)🤖 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/mode/iconpack/conversion/IconPackConversionViewModel.kt` around lines 274 - 332, Sequential exact-then-case-insensitive dedup in resolvedIcons can produce cascading collisions (e.g., "Test" → "Test1" collides with existing "Test1"); update the logic in the IconPackConversionViewModel resolver to repeat the dedupe pass until stable: extract the current per-location dedupe logic (the block producing iconsWithResolvedExactDuplicates and the lowercase pass) into a resolveOnce function that takes a list of processedIcons and returns resolved list, then loop do { resolved = resolveOnce(current); } while (resolved != current) so the resolver (used where resolvedIcons is computed) re-runs until no new renames occur; retain use of IconName and keep checkImportIssues as the final validation step.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kt (1)
44-92: Pack identifier logic is duplicated between exact and case-insensitive checks.Lines 49–55 and 72–78 contain identical
packIdentifiercomputation. Extract it to a helper to keep the code DRY.♻️ Extract pack identifier computation
+private fun BatchIcon.Valid.packIdentifier(useFlatPackage: Boolean): String { + return when (val pack = iconPack) { + is IconPack.Single -> pack.iconPackName + is IconPack.Nested -> when { + useFlatPackage -> pack.iconPackName + else -> "${pack.iconPackName}.${pack.currentNestedPack}" + } + } +} + fun List<BatchIcon>.checkImportIssues(useFlatPackage: Boolean = false): Map<ValidationError, List<IconName>> { return buildMap { // ... existing broken/empty/space checks ... val duplicates = this@checkImportIssues .filterIsInstance<BatchIcon.Valid>() - .groupBy { - val packIdentifier = when (val pack = it.iconPack) { - is IconPack.Single -> pack.iconPackName - is IconPack.Nested -> when { - useFlatPackage -> pack.iconPackName - else -> "${pack.iconPackName}.${pack.currentNestedPack}" - } - } - packIdentifier to it.iconName.name - } + .groupBy { it.packIdentifier(useFlatPackage) to it.iconName.name } // ... val caseInsensitiveDuplicates = this@checkImportIssues .filterIsInstance<BatchIcon.Valid>() - .groupBy { - val packIdentifier = when (val pack = it.iconPack) { - is IconPack.Single -> pack.iconPackName - is IconPack.Nested -> when { - useFlatPackage -> pack.iconPackName - else -> "${pack.iconPackName}.${pack.currentNestedPack}" - } - } - packIdentifier to it.iconName.name.lowercase() - } + .groupBy { it.packIdentifier(useFlatPackage) to it.iconName.name.lowercase() } // ... } }🤖 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/mode/iconpack/conversion/ui/util/IconValidation.kt` around lines 44 - 92, The pack-identifier computation is duplicated in the exact-duplicate and case-insensitive duplicate checks; extract it into a small helper and call it from both groupBy blocks. Add a private helper (e.g., packIdentifier(pack: IconPack, useFlatPackage: Boolean): String or an extension on BatchIcon.Valid) that returns the same string produced by the when(...) logic used in lines with IconPack.Single / IconPack.Nested, then replace the duplicated when blocks in both groupBy lambdas with a call to that helper and keep the rest of the groupBy (pairing with it.iconName.name or it.iconName.name.lowercase()) unchanged.
🤖 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/cli/src/test/kotlin/io/github/composegears/valkyrie/cli/command/SvgXmlToImageVectorCommandTest.kt`:
- Around line 139-177: The test is flaky because it asserts the exact ordering
"TestIcon, Testicon" while directory iteration (listDirectoryEntries) can return
files in either order; update the test in SvgXmlToImageVectorCommandTest (the
test `should throw error for case-insensitive duplicate icon names`) to be
order-independent: replace the exact equality check and strict mock `every {
outputError(mockMessage) }` with assertions that the thrown exception message
contains both "TestIcon" and "Testicon" (e.g., two contains checks or a regex)
and change the `verify { outputError(...) }` to match a string that contains
both names (use a predicate/matcher rather than exact string). Alternatively, if
you prefer production determinism, sort the duplicate icon names before building
the error message in SvgXmlToImageVectorCommand (the code path that detects
case-insensitive duplicates) so the message is always in a stable order.
In
`@tools/gradle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kt`:
- Around line 350-351: The assertion in FailedConfigurationTest.kt is
order-sensitive: instead of asserting the exact substring "TestIcon, Testicon"
on result.output, assert each icon name independently to avoid file-order
variations; replace the single contains("TestIcon, Testicon") check with two
assertions like assertThat(result.output).contains("TestIcon") and
assertThat(result.output).contains("Testicon") (keeping the existing
assertThat(result.output).contains("Found icon names that would collide on
case-insensitive file systems")).
In `@tools/idea-plugin/CHANGELOG.md`:
- Line 10: Update the changelog entry sentence to avoid the split infinitive:
change "Add automatic re-validation when `useFlatPackage` setting changes to
immediately detect new conflicts" to "Add automatic re-validation when
`useFlatPackage` setting changes to detect new conflicts immediately", keeping
the same intent and referencing the `useFlatPackage` setting.
---
Nitpick comments:
In
`@components/parser/unified/src/commonTest/kotlin/io/github/composegears/valkyrie/parser/unified/util/IconNameFormatterTest.kt`:
- Around line 53-65: Rename the test function currently named `check
case-insensitive collision detection` to a clearer name that reflects it asserts
formatter output, e.g. `check formatting that produces case-insensitive
collision`, and update any references; this is the test that calls
`IconNameFormatter.format(fileName)` with `IconTest` cases (the two inputs
"test-icon.svg" and "testicon.svg") and should not be confused with collision
logic exercised elsewhere (`IconValidationTest`, `FailedConfigurationTest`).
In
`@tools/cli/src/main/kotlin/io/github/composegears/valkyrie/cli/command/SvgXmlToImageVectorCommand.kt`:
- Around line 221-251: The duplicate-name error messages are non-deterministic
because exactDuplicates and caseInsensitiveDuplicates derive order from
iconNames; modify the construction or the join step to sort the names before
reporting so output is deterministic for tests—specifically, when building
exactDuplicates and caseInsensitiveDuplicates (symbols: exactDuplicates,
caseInsensitiveDuplicates, iconNames) or immediately before calling outputError,
call a deterministic sort (e.g., sort() on the list) and then joinToString to
produce the error text for both outputError calls.
In
`@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt`:
- Around line 422-455: The inline duplicate detection inside iconsByPack.forEach
duplicates the logic in checkDuplicatesInIconNames; replace the exact- and
case-insensitive checks with a call to checkDuplicatesInIconNames by passing the
computed names list and a context string built from packIdentifier (e.g., "pack
\"$packIdentifier\"" or the same format used elsewhere for consistency). Locate
the block that computes names (IconNameFormatter.format) and remove the two
duplicate-check blocks, invoking checkDuplicatesInIconNames(names, context)
instead so all duplicate handling is centralized in that helper.
- Line 399: The validateDuplicates function currently takes iconNames but
recomputes names for the nested-packs branch using IconNameFormatter.format,
causing double work and leaking caller structure; change the signature of
validateDuplicates to accept a Map<File, String> (precompute icon names once at
the call site using IconNameFormatter.format for each File) or drop the
parameter and always derive names from files inside validateDuplicates, then
update all call sites to either pass the Map<File,String> or let the function
compute names; inside validateDuplicates (and the nestedPacks branch) use the
map lookup (or the single consistent derivation path) instead of calling
IconNameFormatter.format again to eliminate the duplicate computation and remove
the unused iconNames usage.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/textfield/ConfirmTextField.kt`:
- Around line 46-50: The LaunchedEffect(text) block can overwrite an actively
edited value; modify the condition to only apply external text when the text
differs AND the field is not focused (i.e., add a !focused check) so updates
propagate when idle but don't clobber in-progress edits; update the
LaunchedEffect(text) condition that calls state.setTextAndPlaceCursorAtEnd(text)
to include the focused guard.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt`:
- Around line 274-332: Sequential exact-then-case-insensitive dedup in
resolvedIcons can produce cascading collisions (e.g., "Test" → "Test1" collides
with existing "Test1"); update the logic in the IconPackConversionViewModel
resolver to repeat the dedupe pass until stable: extract the current
per-location dedupe logic (the block producing iconsWithResolvedExactDuplicates
and the lowercase pass) into a resolveOnce function that takes a list of
processedIcons and returns resolved list, then loop do { resolved =
resolveOnce(current); } while (resolved != current) so the resolver (used where
resolvedIcons is computed) re-runs until no new renames occur; retain use of
IconName and keep checkImportIssues as the final validation step.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kt`:
- Around line 44-92: The pack-identifier computation is duplicated in the
exact-duplicate and case-insensitive duplicate checks; extract it into a small
helper and call it from both groupBy blocks. Add a private helper (e.g.,
packIdentifier(pack: IconPack, useFlatPackage: Boolean): String or an extension
on BatchIcon.Valid) that returns the same string produced by the when(...) logic
used in lines with IconPack.Single / IconPack.Nested, then replace the
duplicated when blocks in both groupBy lambdas with a call to that helper and
keep the rest of the groupBy (pairing with it.iconName.name or
it.iconName.name.lowercase()) unchanged.
In
`@tools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidationTest.kt`:
- Around line 167-175: The new test `nested without flatPackage, allow exact
duplicates in different nested packs` duplicates the existing `nested, allow
duplicates for different currentNestedPack` test; either remove the older
`nested, allow duplicates for different currentNestedPack` test or instead
change one of them to explicitly document the API difference (e.g., keep the new
test to assert the explicit parameter `useFlatPackage = false` and delete or
rename the older test) so you don't maintain two assertions for the same
invariant; update/remove the redundant test that uses validIcon, nestedPack, and
checkImportIssues to keep a single clear test.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
components/parser/unified/src/commonTest/kotlin/io/github/composegears/valkyrie/parser/unified/util/IconNameFormatterTest.kttools/cli/CHANGELOG.mdtools/cli/src/main/kotlin/io/github/composegears/valkyrie/cli/command/SvgXmlToImageVectorCommand.kttools/cli/src/test/kotlin/io/github/composegears/valkyrie/cli/command/SvgXmlToImageVectorCommandTest.kttools/gradle-plugin/CHANGELOG.mdtools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kttools/gradle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kttools/idea-plugin/CHANGELOG.mdtools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/textfield/ConfirmTextField.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionState.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kttools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidationTest.kt
...rc/test/kotlin/io/github/composegears/valkyrie/cli/command/SvgXmlToImageVectorCommandTest.kt
Show resolved
Hide resolved
...dle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt (1)
421-455: Extract duplicate-check logic to reusecheckDuplicatesInIconNames.The exact-duplicate and case-insensitive-duplicate checks inside this loop (lines 425–454) are a near-verbatim copy of
checkDuplicatesInIconNames. You can eliminate the duplication by delegating to that method:♻️ Proposed refactor
iconsByPack.forEach { (packIdentifier, filesInPack) -> val names = filesInPack.map { IconNameFormatter.format(name = it.name) } - - // Check exact duplicates within this pack/group - val exactDuplicates = names - .groupBy { it } - .filter { it.value.size > 1 } - .keys - .toList() - - if (exactDuplicates.isNotEmpty()) { - throw GradleException( - "Found duplicate icon names in \"$packIdentifier\": ${exactDuplicates.joinToString(", ")}. " + - "Each icon must have a unique name. " + - "Please rename the source files to avoid duplicates.", - ) - } - - // Check case-insensitive duplicates within this pack/group - val caseInsensitiveDuplicates = names - .groupBy { it.lowercase() } - .filter { it.value.size > 1 && it.value.distinct().size > 1 } - .values - .flatten() - .distinct() - - if (caseInsensitiveDuplicates.isNotEmpty()) { - throw GradleException( - "Found icon names that would collide on case-insensitive file systems (macOS/Windows) in \"$packIdentifier\": " + - "${caseInsensitiveDuplicates.joinToString(", ")}. " + - "These icons would overwrite each other during generation. " + - "Please rename the source files to avoid case-insensitive duplicates.", - ) - } + checkDuplicatesInIconNames(names, "\"$packIdentifier\"") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt` around lines 421 - 455, The existing duplicate-detection block inside the iconsByPack.forEach loop duplicates logic already implemented in checkDuplicatesInIconNames; replace the in-loop exact-duplicate and case-insensitive-duplicate code with a call to that helper (pass the pack identifier and the formatted names list produced with IconNameFormatter.format) and remove the local grouping/filtering logic and its GradleException throws; ensure the call is placed where the current checks run (inside the iconsByPack.forEach) so checkDuplicatesInIconNames handles throwing on duplicates for each packIdentifier and names.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kt (1)
44-89: Pack-identifier computation is duplicated verbatim — consider extracting a helper.The block computing
packIdentifierat lines 49–55 (exact duplicates) and lines 72–78 (case-insensitive duplicates) is identical. The same logic also appears a third time inIconPackConversionViewModel.resolveImportIssues(lines 276–282). Extracting a small private extension keeps future changes to the pack-grouping rule in one place.♻️ Suggested extraction
Add a shared private helper, e.g. in a companion file or at the top of
IconValidation.kt:+private fun BatchIcon.Valid.packIdentifier(useFlatPackage: Boolean): String = + when (val pack = iconPack) { + is IconPack.Single -> pack.iconPackName + is IconPack.Nested -> if (useFlatPackage) pack.iconPackName + else "${pack.iconPackName}.${pack.currentNestedPack}" + }Then simplify both
groupBykeys:- .groupBy { - val packIdentifier = when (val pack = it.iconPack) { - is IconPack.Single -> pack.iconPackName - is IconPack.Nested -> when { - useFlatPackage -> pack.iconPackName - else -> "${pack.iconPackName}.${pack.currentNestedPack}" - } - } - packIdentifier to it.iconName.name - } + .groupBy { it.packIdentifier(useFlatPackage) to it.iconName.name }- .groupBy { - val packIdentifier = when (val pack = it.iconPack) { - is IconPack.Single -> pack.iconPackName - is IconPack.Nested -> when { - useFlatPackage -> pack.iconPackName - else -> "${pack.iconPackName}.${pack.currentNestedPack}" - } - } - packIdentifier to it.iconName.name.lowercase() - } + .groupBy { it.packIdentifier(useFlatPackage) to it.iconName.name.lowercase() }🤖 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/mode/iconpack/conversion/ui/util/IconValidation.kt` around lines 44 - 89, The pack-identifier computation is duplicated in IconValidation.checkImportIssues and also in IconPackConversionViewModel.resolveImportIssues; extract a small private helper (e.g., fun BatchIcon.Valid.packGroupKey(useFlatPackage: Boolean): Pair<String, String> or an extension like BatchIcon.Valid.packIdentifier(useFlatPackage)) that returns the same Pair(packIdentifier, iconName) logic using IconPack.Single / IconPack.Nested and useFlatPackage, then replace the two groupBy lambdas in IconValidation (and the similar code in IconPackConversionViewModel.resolveImportIssues) to call that helper so all grouping logic lives in one place and future changes are centralized.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt (2)
274-282: Same pack-identifier logic triplicated here (also appears twice inIconValidation.kt).Consider exposing the helper suggested for
IconValidation.ktfrom a shared location so all three sites stay in sync.🤖 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/mode/iconpack/conversion/IconPackConversionViewModel.kt` around lines 274 - 282, The pack-identification logic used in the processedIcons.groupBy (iconsByLocation) is duplicated with code in IconValidation.kt; extract that when/pack logic into a single shared helper (e.g., an extension function like IconPack.getPackIdentifier(isFlatPackage: Boolean) or a util function getPackIdentifier(iconPack: IconPack, isFlatPackage: Boolean)) and replace the inline when in IconPackConversionViewModel (the processedIcons.groupBy block) and the duplicated occurrences in IconValidation.kt to call the new shared helper so all three sites use the same implementation.
259-259:resolveImportIssueslaunches withoutDispatchers.Default, unlike every other state-mutation function.
deleteIcon,updateIconPack, andrenameIconall useDispatchers.Default. The reworked body ofresolveImportIssuesnow does non-trivial grouping, mapping, and counter bookkeeping over the full icon list, making the inconsistency more pronounced.♻️ Proposed fix
-fun resolveImportIssues() = viewModelScope.launch { +fun resolveImportIssues() = viewModelScope.launch(Dispatchers.Default) {🤖 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/mode/iconpack/conversion/IconPackConversionViewModel.kt` at line 259, The resolveImportIssues function performs non-trivial CPU work but launches on the main dispatcher; update its coroutine launcher to use Dispatchers.Default like deleteIcon, updateIconPack, and renameIcon so heavy grouping/mapping/counter work runs off the main thread. Locate resolveImportIssues in IconPackConversionViewModel and change viewModelScope.launch { ... } to viewModelScope.launch(Dispatchers.Default) { ... } (or use withContext(Dispatchers.Default) inside the existing coroutine) ensuring all state-mutating calls that must run on the main thread are switched back to Dispatchers.Main if needed.
🤖 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/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt`:
- Around line 404-419: In generateIconsForNestedPacks, ensure the same set of
files is used for validation and generation: when nestedPacks.isNotEmpty() and
useFlatPackage is false, filter the incoming files to only those whose parent
folder matches a configured nestedPack.sourceFolder before you build
fileToNestedPack and iconsByPack; update validateDuplicates to operate on that
filtered list (or pass the filtered list into validateDuplicates) so files that
won't be generated are neither validated nor grouped into the root pack.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt`:
- Around line 291-331: The two-stage rename (in IconPackConversionViewModel
where iconsWithResolvedExactDuplicates and the subsequent
lowercaseGroups/lowercaseCounters pass produce IconName("$...${counter - 1}"))
can create new collisions; change the renaming logic to check each newly
generated candidate against the full set of already-committed names (original
names plus any names already assigned in the current pass) and loop/increment
the suffix until the candidate is unique before calling icon.copy(...). Apply
this uniqueness check when assigning names in both the exact-duplicate pass
(nameCounters usage) and the case-insensitive pass (lowercaseCounters usage) so
the method always produces names not present in the current committed set (use a
mutable Set<String> of committedNames updated when you assign a new IconName);
this will ensure convergence and avoid creating new duplicates that break
checkImportIssues.
---
Nitpick comments:
In
`@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt`:
- Around line 421-455: The existing duplicate-detection block inside the
iconsByPack.forEach loop duplicates logic already implemented in
checkDuplicatesInIconNames; replace the in-loop exact-duplicate and
case-insensitive-duplicate code with a call to that helper (pass the pack
identifier and the formatted names list produced with IconNameFormatter.format)
and remove the local grouping/filtering logic and its GradleException throws;
ensure the call is placed where the current checks run (inside the
iconsByPack.forEach) so checkDuplicatesInIconNames handles throwing on
duplicates for each packIdentifier and names.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt`:
- Around line 274-282: The pack-identification logic used in the
processedIcons.groupBy (iconsByLocation) is duplicated with code in
IconValidation.kt; extract that when/pack logic into a single shared helper
(e.g., an extension function like IconPack.getPackIdentifier(isFlatPackage:
Boolean) or a util function getPackIdentifier(iconPack: IconPack, isFlatPackage:
Boolean)) and replace the inline when in IconPackConversionViewModel (the
processedIcons.groupBy block) and the duplicated occurrences in
IconValidation.kt to call the new shared helper so all three sites use the same
implementation.
- Line 259: The resolveImportIssues function performs non-trivial CPU work but
launches on the main dispatcher; update its coroutine launcher to use
Dispatchers.Default like deleteIcon, updateIconPack, and renameIcon so heavy
grouping/mapping/counter work runs off the main thread. Locate
resolveImportIssues in IconPackConversionViewModel and change
viewModelScope.launch { ... } to viewModelScope.launch(Dispatchers.Default) {
... } (or use withContext(Dispatchers.Default) inside the existing coroutine)
ensuring all state-mutating calls that must run on the main thread are switched
back to Dispatchers.Main if needed.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kt`:
- Around line 44-89: The pack-identifier computation is duplicated in
IconValidation.checkImportIssues and also in
IconPackConversionViewModel.resolveImportIssues; extract a small private helper
(e.g., fun BatchIcon.Valid.packGroupKey(useFlatPackage: Boolean): Pair<String,
String> or an extension like BatchIcon.Valid.packIdentifier(useFlatPackage))
that returns the same Pair(packIdentifier, iconName) logic using IconPack.Single
/ IconPack.Nested and useFlatPackage, then replace the two groupBy lambdas in
IconValidation (and the similar code in
IconPackConversionViewModel.resolveImportIssues) to call that helper so all
grouping logic lives in one place and future changes are centralized.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/textfield/ConfirmTextField.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/textfield/ConfirmTextField.kt
...main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt
Show resolved
Hide resolved
...thub/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt
Show resolved
Hide resolved
egorikftp
force-pushed
the
bug/case-insensitive
branch
from
5356322 to
d36fb4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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/mode/iconpack/conversion/IconPackConversionViewModel.kt (1)
259-259:⚠️ Potential issue | 🟠 Major
resolveImportIssuesruns heavy computation on the main thread.
viewModelScope.launch {}with no dispatcher argument inheritsDispatchers.Main.immediate. Every other mutating function in this ViewModel (deleteIcon,updateIconPack,renameIcon,import,processText,processFiles) explicitly usesDispatchers.Defaultfor CPU-bound work. The two-stage deduplication here — multiplegroupBy, nestedmap, and unboundedwhileloops over all icons — should follow the same pattern.🐛 Proposed fix
- fun resolveImportIssues() = viewModelScope.launch { + fun resolveImportIssues() = viewModelScope.launch(Dispatchers.Default) {🤖 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/mode/iconpack/conversion/IconPackConversionViewModel.kt` at line 259, resolveImportIssues currently launches on the main dispatcher and does heavy CPU work; change its coroutine to run on Dispatchers.Default (e.g., viewModelScope.launch(Dispatchers.Default) or wrap heavy sections in withContext(Dispatchers.Default)) so the groupBy/map/while loops run off the main thread, and if you must update UI state or LiveData inside resolveImportIssues ensure those updates are wrapped with withContext(Dispatchers.Main); reference resolveImportIssues, viewModelScope.launch and Dispatchers.Default when making the change.
♻️ Duplicate comments (1)
tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt (1)
408-414:⚠️ Potential issue | 🟡 MinorIncomplete fix: unmatched files still included in validation when
useFlatPackage=true.
generateIconsForNestedPacksalways skips files whose parent directory isn't insourceFolderToNestedPack(theif (nestedPack != null)guard at line 282 is unconditional). The current fix filtersfilesToValidateonly for theuseFlatPackage=falsebranch, so whenuseFlatPackage=true, files outside any configured source folder are still validated against icons that will actually be generated — which can produce a false-positiveGradleException.The filter should be applied unconditionally:
🐛 Proposed fix
- val filesToValidate = if (useFlatPackage) { - files - } else { - files.filter { file -> sourceFolderToNestedPack.containsKey(file.parentFile.name) } - } + val filesToValidate = files.filter { file -> + sourceFolderToNestedPack.containsKey(file.parentFile.name) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt` around lines 408 - 414, The validation currently only filters filesToValidate when useFlatPackage is false, which lets files outside configured nested packs slip through when useFlatPackage=true and produce false-positive GradleException; change the logic so the sourceFolderToNestedPack filter is applied unconditionally when building filesToValidate (regardless of useFlatPackage) — update the filesToValidate computation near useFlatPackage/filesToValidate to always filter by sourceFolderToNestedPack.containsKey(file.parentFile.name) and keep generateIconsForNestedPacks’ existing guard (nestedPack != null) unchanged so validation matches the actual icon generation set.
🧹 Nitpick comments (5)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/textfield/ConfirmTextField.kt (1)
46-50: Guard the sync against active user input by skipping when the field is focused.
focusedis already tracked in this composable. Without the guard, any externaltextchange (e.g., triggered by duplicate-name resolution in the same composition cycle) will callsetTextAndPlaceCursorAtEndwhile the user is mid-edit, discarding their in-progress input. The bug is unlikely in the current use-case (duplicate detection fires only after a commit), but there's no structural protection.♻️ Proposed fix
- // Update text field state when external text changes - LaunchedEffect(text) { - if (state.text.toString() != text) { - state.setTextAndPlaceCursorAtEnd(text) - } - } + // Update text field state when external text changes, but only when + // the user is not actively editing to avoid discarding in-progress input. + LaunchedEffect(text) { + if (!focused && state.text.toString() != text) { + state.setTextAndPlaceCursorAtEnd(text) + } + }🤖 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/jewel/textfield/ConfirmTextField.kt` around lines 46 - 50, The LaunchedEffect(text) sync can overwrite in-progress user edits; change it to skip applying external text updates when the field is currently focused—i.e., inside the LaunchedEffect that compares state.text.toString() and text, first check the composable's focused flag and only call state.setTextAndPlaceCursorAtEnd(text) when focused is false (guard against active user input), keeping the existing equality check and the same setTextAndPlaceCursorAtEnd logic.tools/gradle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kt (1)
337-341:svgContentis copy-pasted identically into all six new test methods.Extract it once at the class or companion-object level.
♻️ Proposed refactor
Add a companion object (or top-level
private val) in the file:class FailedConfigurationTest : CommonGradleTest() { + + companion object { + private val SVG_CONTENT = """ + <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"> + <path d="M0 0h24v24H0z" fill="none"/> + </svg> + """.trimIndent() + }Then replace each inline
val svgContent = """…""".trimIndent()block withSVG_CONTENT.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/gradle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kt` around lines 337 - 341, The six tests duplicate the same svgContent string; extract it into a single shared constant (e.g., a companion object constant SVG_CONTENT or a top-level private val SVG_CONTENT) and replace each inline val svgContent = """...""".trimIndent() in the test methods with references to SVG_CONTENT; update imports or visibility if needed so the test methods can access the constant (put it inside the test class companion object or as a private top-level value).tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt (2)
450-456:distinct().size > 1guard is redundant after the exact-duplicate check.The exact-duplicate block always throws before reaching this line, so any group with
size > 1is already guaranteed to havedistinct().size > 1. The same redundancy exists incheckDuplicatesInIconNames(lines 495–501). It's harmless but may mislead future readers into thinking exact duplicates could reach this branch.♻️ Simplification
- val caseInsensitiveDuplicates = names - .groupBy { it.lowercase() } - .filter { it.value.size > 1 && it.value.distinct().size > 1 } + val caseInsensitiveDuplicates = names + .groupBy { it.lowercase() } + .filter { it.value.size > 1 }Apply the same change at lines 495–497.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt` around lines 450 - 456, The filter condition that checks both it.value.size > 1 and it.value.distinct().size > 1 is redundant because exact-duplicate groups are already handled earlier; update the grouping filter used to compute caseInsensitiveDuplicates (the names.groupBy { it.lowercase() } ... .filter { ... }) to only check it.value.size > 1, and make the identical simplification inside the checkDuplicatesInIconNames function where the same groupBy/filter pattern appears so both places remove the unnecessary distinct().size > 1 guard.
398-474:validateDuplicatessignature is asymmetric across branches.The nested-packs branch uses only
files(and re-callsIconNameFormatter.formatat line 431, duplicating the work from line 108), while the non-nested branches use onlyiconNamesand ignorefiles. Consider either:
- Removing
iconNamesfrom the signature and always deriving names fromfiles(consistent with the nested-packs branch), or- Removing
filesand building the pack-to-name grouping externally (passing a pre-groupedMap<String, List<String>>instead).Either change also eliminates the double call to
IconNameFormatter.formatfor the nested case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt` around lines 398 - 474, The validateDuplicates function currently takes both files and iconNames causing inconsistency and duplicated IconNameFormatter.format calls; change validateDuplicates to accept only files (remove iconNames parameter), derive formatted names from the provided files once (use IconNameFormatter.format in one place), update the non-nested branch to call checkDuplicatesInIconNames(formatNames, "package \"${packageName.get()}\"") or call the existing duplicate-check logic directly with the derived names, and remove the extra formatting at line 431 so nested-pack grouping reuses the already-formatted names; update all callers of validateDuplicates accordingly to pass files only.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kt (1)
44-93: Extract the duplicated pack-identifier lambda into a shared local val.The pack-identifier expression (lines 49–55) is copy-pasted verbatim for the case-insensitive block (lines 73–79). Promote it to a local
val packIdentifier: (BatchIcon.Valid) -> Stringcomputed once before the twogroupBycalls.♻️ Proposed refactor
+ val packIdentifier: (BatchIcon.Valid) -> String = { icon -> + when (val pack = icon.iconPack) { + is IconPack.Single -> pack.iconPackName + is IconPack.Nested -> when { + useFlatPackage -> pack.iconPackName + else -> "${pack.iconPackName}.${pack.currentNestedPack}" + } + } + } + val duplicates = this@checkImportIssues .filterIsInstance<BatchIcon.Valid>() .groupBy { - val packIdentifier = when (val pack = it.iconPack) { - is IconPack.Single -> pack.iconPackName - is IconPack.Nested -> when { - useFlatPackage -> pack.iconPackName - else -> "${pack.iconPackName}.${pack.currentNestedPack}" - } - } - packIdentifier to it.iconName.name + packIdentifier(it) to it.iconName.name } ... val caseInsensitiveDuplicates = this@checkImportIssues .filterIsInstance<BatchIcon.Valid>() .groupBy { - val packIdentifier = when (val pack = it.iconPack) { - is IconPack.Single -> pack.iconPackName - is IconPack.Nested -> when { - useFlatPackage -> pack.iconPackName - else -> "${pack.iconPackName}.${pack.currentNestedPack}" - } - } - packIdentifier to it.iconName.name.lowercase() + packIdentifier(it) to it.iconName.name.lowercase() }🤖 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/mode/iconpack/conversion/ui/util/IconValidation.kt` around lines 44 - 93, Extract the duplicated pack-identifier logic into a single local val (e.g., val packIdentifier: (BatchIcon.Valid) -> String) defined before computing duplicates; implement it to return the same string currently built in both groupBy lambdas (handling IconPack.Single and IconPack.Nested with useFlatPackage and pack.currentNestedPack), then replace the two inline packIdentifier constructions in the groupBy calls with a call to this function (pass the BatchIcon.Valid instance to packIdentifier) so both the exact-duplicate and case-insensitive-duplicate groupBy use the shared logic.
🤖 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/gradle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kt`:
- Around line 582-585: This test currently asserts the general collision message
and the pack identifier (ValkyrieIcons.Outlined) but omits asserting the
individual colliding icon names; update the test that calls failTask(root,
TASK_NAME) to also assert the specific icon names that collide (same pattern as
the other tests that use assertThat(result.output).contains(...)). Add
assertions like
assertThat(result.output).contains("<first-colliding-icon-name>") and
assertThat(result.output).contains("<second-colliding-icon-name>") matching the
actual names used in this test's setup so regressions in reported icon names are
detected.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt`:
- Around line 322-353: The Stage 2 uniqueness check uses
committedNames.contains(candidateName) which is case-sensitive; change this to a
case-insensitive guard by maintaining a parallel set of lowercase committed
names (e.g., committedNamesLower = mutableSetOf<String>() seeded from
committedNames.map { it.lowercase() }) and use
committedNamesLower.contains(candidateName.lowercase()) in the while loop, and
when you add a new candidate to committedNames also add
candidateName.lowercase() to committedNamesLower; update references around
iconsWithResolvedExactDuplicates, lowercaseCounters, IconName creation to use
this new lowercase set so collisions are prevented case-insensitively.
---
Outside diff comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt`:
- Line 259: resolveImportIssues currently launches on the main dispatcher and
does heavy CPU work; change its coroutine to run on Dispatchers.Default (e.g.,
viewModelScope.launch(Dispatchers.Default) or wrap heavy sections in
withContext(Dispatchers.Default)) so the groupBy/map/while loops run off the
main thread, and if you must update UI state or LiveData inside
resolveImportIssues ensure those updates are wrapped with
withContext(Dispatchers.Main); reference resolveImportIssues,
viewModelScope.launch and Dispatchers.Default when making the change.
---
Duplicate comments:
In
`@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt`:
- Around line 408-414: The validation currently only filters filesToValidate
when useFlatPackage is false, which lets files outside configured nested packs
slip through when useFlatPackage=true and produce false-positive
GradleException; change the logic so the sourceFolderToNestedPack filter is
applied unconditionally when building filesToValidate (regardless of
useFlatPackage) — update the filesToValidate computation near
useFlatPackage/filesToValidate to always filter by
sourceFolderToNestedPack.containsKey(file.parentFile.name) and keep
generateIconsForNestedPacks’ existing guard (nestedPack != null) unchanged so
validation matches the actual icon generation set.
---
Nitpick comments:
In
`@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt`:
- Around line 450-456: The filter condition that checks both it.value.size > 1
and it.value.distinct().size > 1 is redundant because exact-duplicate groups are
already handled earlier; update the grouping filter used to compute
caseInsensitiveDuplicates (the names.groupBy { it.lowercase() } ... .filter {
... }) to only check it.value.size > 1, and make the identical simplification
inside the checkDuplicatesInIconNames function where the same groupBy/filter
pattern appears so both places remove the unnecessary distinct().size > 1 guard.
- Around line 398-474: The validateDuplicates function currently takes both
files and iconNames causing inconsistency and duplicated
IconNameFormatter.format calls; change validateDuplicates to accept only files
(remove iconNames parameter), derive formatted names from the provided files
once (use IconNameFormatter.format in one place), update the non-nested branch
to call checkDuplicatesInIconNames(formatNames, "package
\"${packageName.get()}\"") or call the existing duplicate-check logic directly
with the derived names, and remove the extra formatting at line 431 so
nested-pack grouping reuses the already-formatted names; update all callers of
validateDuplicates accordingly to pass files only.
In
`@tools/gradle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kt`:
- Around line 337-341: The six tests duplicate the same svgContent string;
extract it into a single shared constant (e.g., a companion object constant
SVG_CONTENT or a top-level private val SVG_CONTENT) and replace each inline val
svgContent = """...""".trimIndent() in the test methods with references to
SVG_CONTENT; update imports or visibility if needed so the test methods can
access the constant (put it inside the test class companion object or as a
private top-level value).
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/textfield/ConfirmTextField.kt`:
- Around line 46-50: The LaunchedEffect(text) sync can overwrite in-progress
user edits; change it to skip applying external text updates when the field is
currently focused—i.e., inside the LaunchedEffect that compares
state.text.toString() and text, first check the composable's focused flag and
only call state.setTextAndPlaceCursorAtEnd(text) when focused is false (guard
against active user input), keeping the existing equality check and the same
setTextAndPlaceCursorAtEnd logic.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kt`:
- Around line 44-93: Extract the duplicated pack-identifier logic into a single
local val (e.g., val packIdentifier: (BatchIcon.Valid) -> String) defined before
computing duplicates; implement it to return the same string currently built in
both groupBy lambdas (handling IconPack.Single and IconPack.Nested with
useFlatPackage and pack.currentNestedPack), then replace the two inline
packIdentifier constructions in the groupBy calls with a call to this function
(pass the BatchIcon.Valid instance to packIdentifier) so both the
exact-duplicate and case-insensitive-duplicate groupBy use the shared logic.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tools/cli/src/main/kotlin/io/github/composegears/valkyrie/cli/command/SvgXmlToImageVectorCommand.kttools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kttools/gradle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kttools/idea-plugin/CHANGELOG.mdtools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/textfield/ConfirmTextField.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/idea-plugin/CHANGELOG.md
...dle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kt
Show resolved
Hide resolved
...thub/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt
Show resolved
Hide resolved
egorikftp
force-pushed
the
bug/case-insensitive
branch
from
d36fb4f to
99df540
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt (1)
410-414: Past comment addressed — unmatched files are now filtered before grouping.The filter at line 413 ensures files not belonging to any configured nested pack folder are excluded from validation when
useFlatPackageis false, matching the generation behavior ingenerateIconsForNestedPacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt` around lines 410 - 414, The previous review concern has already been addressed by filtering unmatched files before grouping; no functional code change is needed—confirm and remove the duplicate review comment. Specifically, verify the logic around filesToValidate (the useFlatPackage conditional and the files.filter using sourceFolderToNestedPack) matches generateIconsForNestedPacks behavior, then clean up the duplicate comment or review note.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt (1)
306-310: Past comments addressed —committedNamesguard is now case-insensitive.Both Stage 1 (line 306) and Stage 2 (line 340) now use
committedNames.any { it.equals(candidateName, ignoreCase = true) }, which prevents the convergence and case-collision issues raised in the previous review.Also applies to: 340-344
🤖 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/mode/iconpack/conversion/IconPackConversionViewModel.kt` around lines 306 - 310, The uniqueness check must be consistently case-insensitive in both stages: update the Stage 2 loop (the code around the second occurrence that manipulates committedNames, candidateName, originalName and suffix) to use committedNames.any { it.equals(candidateName, ignoreCase = true) } (matching Stage 1) when deciding to increment suffix, and then add the chosen candidateName to committedNames; this ensures both the Stage 1 and Stage 2 uniqueness loops use the same case-insensitive comparison and prevent case-collision.tools/gradle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kt (1)
582-587: Past comment addressed — icon name assertions now present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/gradle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kt` around lines 582 - 587, The reviewer note indicates the previous request was already addressed (the test now asserts the colliding icon names), so remove the duplicate review marker and mark the comment as resolved; keep the assertions in FailedConfigurationTest.kt (the failTask(root, TASK_NAME) call and subsequent assertions for "ValkyrieIcons.Outlined", "TestIcon", and "Testicon") as-is and simply clear the duplicate "[duplicate_comment]"/resolve the review thread.
🧹 Nitpick comments (3)
tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt (1)
430-466: Consider extracting duplicate-detection logic to reduce duplication.The exact-duplicate and case-insensitive-duplicate checks in the per-group loop (lines 434–465) are nearly identical to
checkDuplicatesInIconNames(lines 479–510). The only difference is how the error message context string is formatted. You could unify them by having the loop callcheckDuplicatesInIconNames(names, "\"$packIdentifier\"")directly.♻️ Suggested consolidation
iconsByPack.forEach { (packIdentifier, filesInPack) -> val names = filesInPack.map { IconNameFormatter.format(name = it.name) } - - // Check exact duplicates within this pack/group - val exactDuplicates = names - .groupBy { it } - .filter { it.value.size > 1 } - .keys - .toList() - .sorted() - - if (exactDuplicates.isNotEmpty()) { - throw GradleException( - "Found duplicate icon names in \"$packIdentifier\": ${exactDuplicates.joinToString(", ")}. " + - "Each icon must have a unique name. " + - "Please rename the source files to avoid duplicates.", - ) - } - - // Check case-insensitive duplicates within this pack/group - val caseInsensitiveDuplicates = names - .groupBy { it.lowercase() } - .filter { it.value.size > 1 && it.value.distinct().size > 1 } - .values - .flatten() - .distinct() - .sorted() - - if (caseInsensitiveDuplicates.isNotEmpty()) { - throw GradleException( - "Found icon names that would collide on case-insensitive file systems (macOS/Windows) in \"$packIdentifier\": " + - "${caseInsensitiveDuplicates.joinToString(", ")}. " + - "These icons would overwrite each other during generation. " + - "Please rename the source files to avoid case-insensitive duplicates.", - ) - } + checkDuplicatesInIconNames(names, "\"$packIdentifier\"") }Also applies to: 477-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt` around lines 430 - 466, The duplicate-detection code inside the iconsByPack.forEach (where you build names = filesInPack.map { IconNameFormatter.format(...) }) duplicates the logic implemented in checkDuplicatesInIconNames; replace the in-loop exact and case-insensitive checks with a single call to checkDuplicatesInIconNames(names, "\"$packIdentifier\"") (or add a context param to checkDuplicatesInIconNames if needed) so the existing function emits the same package-specific error messages and avoids repeating the grouping/filtering logic.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kt (1)
44-93: Consider extracting the shared pack-identifier logic.The pack-identifier resolution (lines 49–55 and 73–79) is duplicated between the exact-duplicate and case-insensitive-duplicate blocks. A small local function would keep them in sync and reduce the surface for future drift.
♻️ Suggested extraction
fun List<BatchIcon>.checkImportIssues(useFlatPackage: Boolean = false): Map<ValidationError, List<IconName>> { return buildMap { // ... existing code for brokenIcons, empty names, spaces ... fun BatchIcon.Valid.packIdentifier(): String = when (val pack = iconPack) { is IconPack.Single -> pack.iconPackName is IconPack.Nested -> when { useFlatPackage -> pack.iconPackName else -> "${pack.iconPackName}.${pack.currentNestedPack}" } } val duplicates = this@checkImportIssues .filterIsInstance<BatchIcon.Valid>() .groupBy { it.packIdentifier() to it.iconName.name } // ... rest unchanged val caseInsensitiveDuplicates = this@checkImportIssues .filterIsInstance<BatchIcon.Valid>() .groupBy { it.packIdentifier() to it.iconName.name.lowercase() } // ... rest unchanged } }🤖 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/mode/iconpack/conversion/ui/util/IconValidation.kt` around lines 44 - 93, Extract the duplicated pack-identifier logic into a small local function inside checkImportIssues (e.g., fun BatchIcon.Valid.packIdentifier(): String) that returns the same string used currently for IconPack.Single and IconPack.Nested (respecting useFlatPackage and "${pack.iconPackName}.${pack.currentNestedPack}" when not flat); then replace the two inline groupBy lambdas in the duplicates and caseInsensitiveDuplicates blocks to call it (groupBy { it.packIdentifier() to it.iconName.name } and groupBy { it.packIdentifier() to it.iconName.name.lowercase() }) so the logic is centralized and consistent.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt (1)
274-354: Two-stage deduplication logic is correct but has O(n²) characteristic percommittedNames.anycall.Each
while (committedNames.any { it.equals(candidateName, ignoreCase = true) })performs a linear scan of the committed set. With large icon sets this could add up (quadratic in the worst case with many collisions). For typical batch sizes this won't be noticeable, but if you ever expect very large icon packs, consider using a parallelHashSetof lowercased names for O(1) lookups:♻️ Suggested optimization
val resolvedIcons = iconsByLocation.flatMap { (_, iconsInLocation) -> val committedNames = mutableSetOf<String>() + val committedNamesLower = mutableSetOf<String>() + + fun commitName(name: String) { + committedNames.add(name) + committedNamesLower.add(name.lowercase()) + } + + fun isCommitted(name: String): Boolean = name.lowercase() in committedNamesLower // ... in while loops: - while (committedNames.any { it.equals(candidateName, ignoreCase = true) }) { + while (isCommitted(candidateName)) {🤖 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/mode/iconpack/conversion/IconPackConversionViewModel.kt` around lines 274 - 354, The deduplication uses committedNames.any { it.equals(..., ignoreCase = true) } causing O(n²) behavior; replace those linear checks by maintaining a parallel HashSet<String> (e.g., committedLowercaseNames) of lowercase names and use committedLowercaseNames.contains(candidateName.lowercase()) for O(1) lookups, and whenever you add to committedNames (in the exact-duplicate pass and in the case-insensitive pass) also add the lowercase form to committedLowercaseNames; update logic in the block that builds iconsWithResolvedExactDuplicates and the subsequent case-insensitive pass (references: committedNames, iconsWithResolvedExactDuplicates, lowercaseCounters, and the candidateName generation) to use committedLowercaseNames for membership checks and inserts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt`:
- Around line 410-414: The previous review concern has already been addressed by
filtering unmatched files before grouping; no functional code change is
needed—confirm and remove the duplicate review comment. Specifically, verify the
logic around filesToValidate (the useFlatPackage conditional and the
files.filter using sourceFolderToNestedPack) matches generateIconsForNestedPacks
behavior, then clean up the duplicate comment or review note.
In
`@tools/gradle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kt`:
- Around line 582-587: The reviewer note indicates the previous request was
already addressed (the test now asserts the colliding icon names), so remove the
duplicate review marker and mark the comment as resolved; keep the assertions in
FailedConfigurationTest.kt (the failTask(root, TASK_NAME) call and subsequent
assertions for "ValkyrieIcons.Outlined", "TestIcon", and "Testicon") as-is and
simply clear the duplicate "[duplicate_comment]"/resolve the review thread.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt`:
- Around line 306-310: The uniqueness check must be consistently
case-insensitive in both stages: update the Stage 2 loop (the code around the
second occurrence that manipulates committedNames, candidateName, originalName
and suffix) to use committedNames.any { it.equals(candidateName, ignoreCase =
true) } (matching Stage 1) when deciding to increment suffix, and then add the
chosen candidateName to committedNames; this ensures both the Stage 1 and Stage
2 uniqueness loops use the same case-insensitive comparison and prevent
case-collision.
---
Nitpick comments:
In
`@tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt`:
- Around line 430-466: The duplicate-detection code inside the
iconsByPack.forEach (where you build names = filesInPack.map {
IconNameFormatter.format(...) }) duplicates the logic implemented in
checkDuplicatesInIconNames; replace the in-loop exact and case-insensitive
checks with a single call to checkDuplicatesInIconNames(names,
"\"$packIdentifier\"") (or add a context param to checkDuplicatesInIconNames if
needed) so the existing function emits the same package-specific error messages
and avoids repeating the grouping/filtering logic.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kt`:
- Around line 274-354: The deduplication uses committedNames.any {
it.equals(..., ignoreCase = true) } causing O(n²) behavior; replace those linear
checks by maintaining a parallel HashSet<String> (e.g., committedLowercaseNames)
of lowercase names and use
committedLowercaseNames.contains(candidateName.lowercase()) for O(1) lookups,
and whenever you add to committedNames (in the exact-duplicate pass and in the
case-insensitive pass) also add the lowercase form to committedLowercaseNames;
update logic in the block that builds iconsWithResolvedExactDuplicates and the
subsequent case-insensitive pass (references: committedNames,
iconsWithResolvedExactDuplicates, lowercaseCounters, and the candidateName
generation) to use committedLowercaseNames for membership checks and inserts.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kt`:
- Around line 44-93: Extract the duplicated pack-identifier logic into a small
local function inside checkImportIssues (e.g., fun
BatchIcon.Valid.packIdentifier(): String) that returns the same string used
currently for IconPack.Single and IconPack.Nested (respecting useFlatPackage and
"${pack.iconPackName}.${pack.currentNestedPack}" when not flat); then replace
the two inline groupBy lambdas in the duplicates and caseInsensitiveDuplicates
blocks to call it (groupBy { it.packIdentifier() to it.iconName.name } and
groupBy { it.packIdentifier() to it.iconName.name.lowercase() }) so the logic is
centralized and consistent.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tools/cli/src/main/kotlin/io/github/composegears/valkyrie/cli/command/SvgXmlToImageVectorCommand.kttools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kttools/gradle-plugin/src/test/kotlin/io/github/composegears/valkyrie/gradle/FailedConfigurationTest.kttools/idea-plugin/CHANGELOG.mdtools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/textfield/ConfirmTextField.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/IconPackConversionViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/conversion/ui/util/IconValidation.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/textfield/ConfirmTextField.kt
- tools/cli/src/main/kotlin/io/github/composegears/valkyrie/cli/command/SvgXmlToImageVectorCommand.kt
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
1 participant
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-02-23.at.11.22.37.mov
📝 Changelog
If this PR introduces user-facing changes, please update the relevant Unreleased section in changelogs: