Conversation
WalkthroughThis pull request refactors the IntelliJ IDE plugin's icon pack setup flows. It removes the 🚥 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: 4
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/common/IconPackEditor.kt (1)
76-90:⚠️ Potential issue | 🟡 Minor
allowEditingnot guarding the add-button in the empty-packs branch.When
nestedPacks.isEmpty(),AddPackButtonis rendered unconditionally (lines 78–81), but whennestedPacks.isNotEmpty(), the equivalent button insideNestedPacksis gated behindallowEditing(line 135). The Material pack flow will typically start with an empty nested-pack list, so if it setsallowEditing = false, the "Add nested pack" button would still appear — a functional regression in that flow.🐛 Proposed fix
- if (inputFieldState.nestedPacks.isEmpty()) { - Spacer(8.dp) - AddPackButton( - modifier = Modifier.align(Alignment.Start), - onClick = onAddNestedPack, - ) - } else { + if (inputFieldState.nestedPacks.isNotEmpty()) { NestedPacks( nestedPacks = inputFieldState.nestedPacks, allowEditing = inputFieldState.allowEditing, onRemove = onRemoveNestedPack, onValueChange = onValueChange, onAddNestedPack = onAddNestedPack, ) + } else if (inputFieldState.allowEditing) { + Spacer(8.dp) + AddPackButton( + modifier = Modifier.align(Alignment.Start), + onClick = onAddNestedPack, + ) }🤖 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/common/IconPackEditor.kt` around lines 76 - 90, The empty-branch renders AddPackButton unconditionally which bypasses the allowEditing guard used inside NestedPacks; update the empty-packs branch in IconPackEditor so AddPackButton is only shown when inputFieldState.allowEditing is true (e.g., wrap the Spacer + AddPackButton rendering in a check for inputFieldState.allowEditing or render a disabled/hidden placeholder when false) so behavior matches the NestedPacks path; reference the AddPackButton and inputFieldState.allowEditing in your change.
🧹 Nitpick comments (7)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/inputhandler/InputHandler.kt (1)
30-33: Pre-existing:nestedPacks.size.toString()can produce duplicate IDs.Scenario: add (id=
"0"), add (id="1"), remove"0", add → id="1"again. Two packs share"1", andremoveNestedPack(which usesfilterNot { it.id == nestedPack.id }) would then silently delete both.The
onAddNestedPacklambda in theIconPackEditorPreviewalready usesRandom.nextLong().toString()to avoid this. Consider aligningBasicInputHandlerwith the same approach or using a UUID.♻️ Suggested fix
override fun addNestedPack() { _state.updateState { copy( nestedPacks = nestedPacks + NestedPack( - id = nestedPacks.size.toString(), + id = java.util.UUID.randomUUID().toString(), inputFieldState = InputState(), ), ) } }🤖 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/common/inputhandler/InputHandler.kt` around lines 30 - 33, The nested pack ID generation in BasicInputHandler currently uses nestedPacks.size.toString(), which can create duplicate IDs after removals; change the creation of NestedPack in the onAddNestedPack flow to use a unique ID generator (match IconPackEditorPreview) such as Random.nextLong().toString() or UUID.randomUUID().toString() instead of nestedPacks.size.toString() so each NestedPack.id is globally unique and removeNestedPack (which filters by id) will only remove the intended pack.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/ui/MaterialPackCreation.kt (1)
51-51: Consider a dedicated string key for the Material Pack save action.
"iconpack.newpack.creation.continue"is semantically scoped to the NewPack flow. Reusing it works today since the button text is identical, but the namespacing will mislead if the two flows ever need different labels.♻️ Suggested string key
- Text(text = stringResource("iconpack.newpack.creation.continue")) + Text(text = stringResource("iconpack.materialpack.creation.save"))Add a corresponding entry in the bundle:
iconpack.materialpack.creation.save=Save(or whatever the final copy is)
🤖 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/material/ui/MaterialPackCreation.kt` at line 51, Replace the reused NewPack string key with a dedicated Material Pack save key: add a new resource entry (e.g. "iconpack.materialpack.creation.save" -> "Save" or final copy) to the strings bundle, then update the Text composable call that currently uses stringResource("iconpack.newpack.creation.continue") to use stringResource("iconpack.materialpack.creation.save") instead; locate the usage in MaterialPackCreation (the Text/stringResource call) and ensure the new key is added to the appropriate localization file.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackViewModel.kt (1)
42-43: Consider using a bufferedChannelfor event delivery robustness
Channel<NewPackEvent>()defaults toRENDEZVOUScapacity (0), meaningsendsuspends until a receiver is active. WhileLaunchedEffect(Unit)in the screen starts collecting quickly, a brief window during recomposition or initial composition could leave the coroutine stuck on_events.send(...)until the collector resumes.♻️ Proposed change
- private val _events = Channel<NewPackEvent>() + private val _events = Channel<NewPackEvent>(Channel.BUFFERED)🤖 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/newpack/NewPackViewModel.kt` around lines 42 - 43, The Channel used for event delivery (_events of type Channel<NewPackEvent> in NewPackViewModel) is rendezvous by default and can suspend senders during brief gaps in collection; change its construction to a buffered channel (for example using Channel(Channel.BUFFERED) or a small fixed capacity like Channel(1)) so _events.send(...) won't suspend if the collector is not immediately ready, and keep exposing events via events = _events.receiveAsFlow().tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/IconPackDirectoryPicker.kt (1)
42-42: Minor style inconsistency: preferrememberMutableStateoverremember { mutableStateOf(...) }
ChooseExistingPackFile.kt(line 48) uses the project'srememberMutableStatehelper for the sameisDraggingpattern. Using it here keeps the codebase consistent.♻️ Proposed change
- val isDragging by remember(dragAndDropHandler.isDragging) { mutableStateOf(dragAndDropHandler.isDragging) } + val isDragging by rememberMutableState(dragAndDropHandler.isDragging) { dragAndDropHandler.isDragging }Also add to the imports:
+import io.github.composegears.valkyrie.sdk.compose.foundation.rememberMutableState🤖 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/common/IconPackDirectoryPicker.kt` at line 42, Replace the manual remember { mutableStateOf(...) } usage for isDragging with the project's helper: use rememberMutableState(dragAndDropHandler.isDragging) in IconPackDirectoryPicker (replace the line defining val isDragging by ...), and add the corresponding import for rememberMutableState to the file so it matches the pattern used in ChooseExistingPackFile.kt.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/IconPackModeScreen.kt (1)
86-92: Use a more semantically appropriate icon for the Material Pack option
AllIconsKeys.FileTypes.SourceMapis designed for JavaScript source map files and doesn't communicate "Material Design icons" to users. However, reviewing the available IntelliJ platform icons inAllIconsKeys, there are no palette, layers, or design-themed alternatives. Consider usingAllIconsKeys.General.SettingsorAllIconsKeys.Actions.Propertiesas an interim solution, or explore if custom icons can be added to represent Material Design more intuitively.🤖 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/IconPackModeScreen.kt` around lines 86 - 92, Replace the semantically incorrect icon AllIconsKeys.FileTypes.SourceMap used in the InfoCard for the Material Pack option (InfoCard with title stringResource("iconpack.mode.material.pack.title") and onClick = onMaterialPack) with a more appropriate interim IntelliJ icon such as AllIconsKeys.General.Settings or AllIconsKeys.Actions.Properties; update the icon reference passed to the InfoCard's key parameter and ensure the change is applied where InfoCard(...) for the Material Pack is declared so the UI better communicates "Material Design" to users (consider adding a TODO to revisit and supply a custom Material icon later).tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackScreen.kt (1)
43-53: Optional: Simplify event collection inLaunchedEffect.
flow.onEach { }.launchIn(this)inside aLaunchedEffectlaunches a redundant nested coroutine — the LaunchedEffect block itself becomes a fire-and-forget launcher. A directcollectis cleaner and avoids the extra coroutine layer:♻️ Proposed simplification
LaunchedEffect(Unit) { - viewModel.events - .onEach { - when (it) { - is MaterialPackEvent.FinishSetup -> { - navController.replace(dest = IconPackConversionScreen) - } - } - } - .launchIn(this) + viewModel.events.collect { + when (it) { + is MaterialPackEvent.FinishSetup -> navController.replace(dest = IconPackConversionScreen) + } + } }🤖 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/material/MaterialPackScreen.kt` around lines 43 - 53, The LaunchedEffect is using viewModel.events.onEach { ... }.launchIn(this), which spawns a redundant nested coroutine; replace that pattern by directly collecting the flow inside the LaunchedEffect: call viewModel.events.collect { when (it) { is MaterialPackEvent.FinishSetup -> navController.replace(dest = IconPackConversionScreen) } } so you remove onEach(...).launchIn(this) and use collect within the existing LaunchedEffect to simplify and avoid the extra coroutine layer.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackViewModel.kt (1)
39-40: Consider a bufferedChannelfor the events channel.
Channel()defaults toRENDEZVOUS(capacity = 0):sendsuspends until a receiver is ready. IfsaveIconPack()races to sendFinishSetupbeforeLaunchedEffectstarts collecting (e.g., during recomposition), the sending coroutine inviewModelScopewill hang indefinitely.ExistingPackViewModelusesMutableSharedFlowinstead — consider aligning or usingChannel(Channel.BUFFERED).♻️ Proposed fix
-private val _events = Channel<MaterialPackEvent>() +private val _events = Channel<MaterialPackEvent>(Channel.BUFFERED)🤖 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/material/MaterialPackViewModel.kt` around lines 39 - 40, The events channel uses an unbuffered Channel (private val _events = Channel<MaterialPackEvent>), which can suspend senders (e.g., in saveIconPack() running in viewModelScope) if no collector (LaunchedEffect) is ready; change this to a buffered channel or align with ExistingPackViewModel by replacing the channel with Channel(Channel.BUFFERED) (or switch to a MutableSharedFlow/SharedFlow) so sending FinishSetup (MaterialPackEvent) cannot hang, and keep the public events as _events.receiveAsFlow() to preserve the API.
🤖 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/CHANGELOG.md`:
- Line 8: Change the past-tense entry "Added separate \"Material pack\" option"
to imperative mood to match the rest of the changelog; replace that text with
"Add separate \"Material pack\" option" so the entry uses the same verb tense as
the "Add ..." entries.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/tooling/PreviewTheme.kt`:
- Around line 36-37: Move the side-effectful calls out of the composable body
and into a DisposableEffect: call GlobalPreviewState.isPreview = true and
DI.initWith(project) inside DisposableEffect tied to project (or Unit) and in
its onDispose set GlobalPreviewState.isPreview = false (and optionally perform
any DI cleanup if available); this ensures DI.initWith(project) runs under
Compose effect tracking and GlobalPreviewState.isPreview is reset on disposal.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/existingpack/ExistingPackViewModel.kt`:
- Around line 119-130: The call to IconPackWriter.savePack in
ExistingPackViewModel is relying on the default writeToFile=true, which creates
an implicit divergence from MaterialPackViewModel (which passes
writeToFile=false); update the IconPackWriter.savePack invocation inside
ExistingPackViewModel.viewModelScope.launch to explicitly pass writeToFile =
true (and do the same in NewPackViewModel where applicable) so the behavior is
explicit and consistent with MaterialPackViewModel's explicit writeToFile=false
usage.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackViewModel.kt`:
- Line 40: Change the mutable property inputHandler to an immutable val since
it's only initialized once and never reassigned; locate the declaration "private
var inputHandler = BasicInputHandler(InputFieldState.Empty)" in NewPackViewModel
and replace var with val so you keep the same BasicInputHandler instance while
retaining existing method calls and property accesses on inputHandler.
---
Outside diff comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/IconPackEditor.kt`:
- Around line 76-90: The empty-branch renders AddPackButton unconditionally
which bypasses the allowEditing guard used inside NestedPacks; update the
empty-packs branch in IconPackEditor so AddPackButton is only shown when
inputFieldState.allowEditing is true (e.g., wrap the Spacer + AddPackButton
rendering in a check for inputFieldState.allowEditing or render a
disabled/hidden placeholder when false) so behavior matches the NestedPacks
path; reference the AddPackButton and inputFieldState.allowEditing in your
change.
---
Nitpick comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/IconPackDirectoryPicker.kt`:
- Line 42: Replace the manual remember { mutableStateOf(...) } usage for
isDragging with the project's helper: use
rememberMutableState(dragAndDropHandler.isDragging) in IconPackDirectoryPicker
(replace the line defining val isDragging by ...), and add the corresponding
import for rememberMutableState to the file so it matches the pattern used in
ChooseExistingPackFile.kt.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/inputhandler/InputHandler.kt`:
- Around line 30-33: The nested pack ID generation in BasicInputHandler
currently uses nestedPacks.size.toString(), which can create duplicate IDs after
removals; change the creation of NestedPack in the onAddNestedPack flow to use a
unique ID generator (match IconPackEditorPreview) such as
Random.nextLong().toString() or UUID.randomUUID().toString() instead of
nestedPacks.size.toString() so each NestedPack.id is globally unique and
removeNestedPack (which filters by id) will only remove the intended pack.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/IconPackModeScreen.kt`:
- Around line 86-92: Replace the semantically incorrect icon
AllIconsKeys.FileTypes.SourceMap used in the InfoCard for the Material Pack
option (InfoCard with title stringResource("iconpack.mode.material.pack.title")
and onClick = onMaterialPack) with a more appropriate interim IntelliJ icon such
as AllIconsKeys.General.Settings or AllIconsKeys.Actions.Properties; update the
icon reference passed to the InfoCard's key parameter and ensure the change is
applied where InfoCard(...) for the Material Pack is declared so the UI better
communicates "Material Design" to users (consider adding a TODO to revisit and
supply a custom Material icon later).
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackScreen.kt`:
- Around line 43-53: The LaunchedEffect is using viewModel.events.onEach { ...
}.launchIn(this), which spawns a redundant nested coroutine; replace that
pattern by directly collecting the flow inside the LaunchedEffect: call
viewModel.events.collect { when (it) { is MaterialPackEvent.FinishSetup ->
navController.replace(dest = IconPackConversionScreen) } } so you remove
onEach(...).launchIn(this) and use collect within the existing LaunchedEffect to
simplify and avoid the extra coroutine layer.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackViewModel.kt`:
- Around line 39-40: The events channel uses an unbuffered Channel (private val
_events = Channel<MaterialPackEvent>), which can suspend senders (e.g., in
saveIconPack() running in viewModelScope) if no collector (LaunchedEffect) is
ready; change this to a buffered channel or align with ExistingPackViewModel by
replacing the channel with Channel(Channel.BUFFERED) (or switch to a
MutableSharedFlow/SharedFlow) so sending FinishSetup (MaterialPackEvent) cannot
hang, and keep the public events as _events.receiveAsFlow() to preserve the API.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/ui/MaterialPackCreation.kt`:
- Line 51: Replace the reused NewPack string key with a dedicated Material Pack
save key: add a new resource entry (e.g. "iconpack.materialpack.creation.save"
-> "Save" or final copy) to the strings bundle, then update the Text composable
call that currently uses stringResource("iconpack.newpack.creation.continue") to
use stringResource("iconpack.materialpack.creation.save") instead; locate the
usage in MaterialPackCreation (the Text/stringResource call) and ensure the new
key is added to the appropriate localization file.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackViewModel.kt`:
- Around line 42-43: The Channel used for event delivery (_events of type
Channel<NewPackEvent> in NewPackViewModel) is rendezvous by default and can
suspend senders during brief gaps in collection; change its construction to a
buffered channel (for example using Channel(Channel.BUFFERED) or a small fixed
capacity like Channel(1)) so _events.send(...) won't suspend if the collector is
not immediately ready, and keep exposing events via events =
_events.receiveAsFlow().
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/tooling/PreviewTheme.kt
Show resolved
Hide resolved
...o/github/composegears/valkyrie/ui/screen/mode/iconpack/existingpack/ExistingPackViewModel.kt
Show resolved
Hide resolved
...n/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackViewModel.kt
Outdated
Show resolved
Hide resolved
egorikftp
force-pushed
the
feature/material-pack
branch
from
fb82121 to
f56af14
Compare
egorikftp
force-pushed
the
feature/material-pack
branch
from
f56af14 to
88d872c
Compare
|
@coderabbitai review all |
|
✅ Actions performedReview triggered.
|
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/common/IconPackEditor.kt (1)
76-90:⚠️ Potential issue | 🟡 MinorCode inconsistency:
allowEditingnot checked whennestedPacksis empty.When
nestedPacksis empty (line 76),AddPackButtonis rendered without checkinginputFieldState.allowEditing. WhennestedPacksis non-empty,allowEditinggates the button (line 135). While Material Pack always pre-populates nested packs (avoiding this scenario), the inconsistency leaves the code fragile. For consistency and defensive coding, apply the check in both branches.Proposed fix
if (inputFieldState.nestedPacks.isEmpty()) { - Spacer(8.dp) - AddPackButton( - modifier = Modifier.align(Alignment.Start), - onClick = onAddNestedPack, - ) + if (inputFieldState.allowEditing) { + Spacer(8.dp) + AddPackButton( + modifier = Modifier.align(Alignment.Start), + onClick = onAddNestedPack, + ) + } } else {🤖 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/common/IconPackEditor.kt` around lines 76 - 90, The branch that renders AddPackButton when inputFieldState.nestedPacks.isEmpty() should also respect inputFieldState.allowEditing; change the empty-branch to only render AddPackButton when inputFieldState.allowEditing is true (same gating used when nestedPacks is non-empty) so AddPackButton is not shown or clickable when editing is disallowed, keeping behavior consistent between the empty and non-empty branches; update the conditional around AddPackButton (referencing inputFieldState, allowEditing, AddPackButton, onAddNestedPack, and NestedPacks) accordingly.
🧹 Nitpick comments (6)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialInputHandler.kt (1)
26-32: Consider using semantic IDs forNestedPackinstead of sequential integers.The IDs
"0"–"4"are unambiguous since the list is static and non-removable, but using the icon set name as the ID (matching thetextvalue) is more self-documenting and easier to reason about if IDs surface in logs or error paths.♻️ Suggested refactor
- NestedPack(id = "0", inputFieldState = InputState(text = "Filled", enabled = false)), - NestedPack(id = "1", inputFieldState = InputState(text = "Outlined", enabled = false)), - NestedPack(id = "2", inputFieldState = InputState(text = "Rounded", enabled = false)), - NestedPack(id = "3", inputFieldState = InputState(text = "TwoTone", enabled = false)), - NestedPack(id = "4", inputFieldState = InputState(text = "Sharp", enabled = false)), + NestedPack(id = "filled", inputFieldState = InputState(text = "Filled", enabled = false)), + NestedPack(id = "outlined", inputFieldState = InputState(text = "Outlined", enabled = false)), + NestedPack(id = "rounded", inputFieldState = InputState(text = "Rounded", enabled = false)), + NestedPack(id = "twoTone", inputFieldState = InputState(text = "TwoTone", enabled = false)), + NestedPack(id = "sharp", inputFieldState = InputState(text = "Sharp", enabled = false)),🤖 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/material/MaterialInputHandler.kt` around lines 26 - 32, Replace the numeric string IDs in the nestedPacks list with semantic IDs that match the icon set names; update the NestedPack entries inside nestedPacks (the list containing NestedPack(...) with inputFieldState = InputState(...)) so each NestedPack.id uses the corresponding text value ("Filled", "Outlined", "Rounded", "TwoTone", "Sharp") instead of "0".."4" to make IDs self-descriptive and match InputState.text.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackScreen.kt (1)
134-141: Preview duplicates theDirectoryStateinitialization literal.The same
DirectoryState(iconPackDestination = "path/to/import", predictedPackage = "com.example.iconpack", nextAvailable = true)appears in both the initial state (line 136) and theonBackhandler (line 180). Consider extracting a preview constant to avoid drift between the two.Also applies to: 179-185
🤖 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/newpack/NewPackScreen.kt` around lines 134 - 141, The DirectoryState literal is duplicated in the initial state and the onBack handler; extract it to a shared immutable preview constant (e.g., PREVIEW_DIRECTORY_STATE or previewDirectoryState) and use that constant when constructing ChooseDestinationDirectoryState in rememberMutableState and inside the onBack handler; update references to DirectoryState(iconPackDestination = "path/to/import", predictedPackage = "com.example.iconpack", nextAvailable = true) so both NewPackModeState creation and the onBack use the single constant (refer to rememberMutableState, ChooseDestinationDirectoryState, DirectoryState, and the onBack handler).tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/existingpack/ExistingPackViewModel.kt (1)
49-50:MutableSharedFlowfor events is inconsistent with sibling ViewModels and can silently drop events.
NewPackViewModelandMaterialPackViewModelwere migrated toChannel + receiveAsFlow()in this PR, which suspendssend()until the collector is ready — guaranteeing delivery.ExistingPackViewModelstill usesMutableSharedFlow(replay=0), meaningemit()completes immediately even with no subscriber, potentially dropping navigation events (e.g.,OnSettingsUpdated,PreviewIconPackObject) if the UI collector is transiently unsubscribed.♻️ Suggested fix
- private val _events = MutableSharedFlow<ExistingPackEvent>() - val events = _events.asSharedFlow() + private val _events = Channel<ExistingPackEvent>() + val events = _events.receiveAsFlow()And replace all
_events.emit(...)calls with_events.send(...).🤖 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/existingpack/ExistingPackViewModel.kt` around lines 49 - 50, Replace the MutableSharedFlow-based event bus in ExistingPackViewModel with a rendezvous Channel to match siblings: change the backing _events from MutableSharedFlow<ExistingPackEvent>() to a Channel<ExistingPackEvent>(Channel.RENDEZVOUS) (or simply Channel()), expose events as _events.receiveAsFlow(), and update all places that call _events.emit(...) to use _events.send(...) so sends suspend until a collector is ready; refer to symbols ExistingPackViewModel, _events, events, and ExistingPackEvent when making the edits.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/ui/MaterialPackCreation.kt (2)
44-46: No-oponAddNestedPack/onRemoveNestedPackare safe but should be guarded if the state could have emptynestedPacks.When
inputFieldState.nestedPacks.isEmpty(),IconPackEditorshowsAddPackButtonwired toonAddNestedPack = {}— clicking it silently does nothing. For the current Material pack state this is unreachable (Material icons always have nested packs), but it's a latent UX dead-end if the state ever has empty packs.🤖 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/material/ui/MaterialPackCreation.kt` around lines 44 - 46, The IconPackEditor is being passed no-op lambdas for onAddNestedPack/onRemoveNestedPack which leads to a dead-end when inputFieldState.nestedPacks.isEmpty(); update MaterialPackCreation so it guards/handles empty nestedPacks by providing a real handler or conditional UI: either supply a concrete onAddNestedPack implementation that inserts a default nested pack into inputFieldState.nestedPacks (or calls the existing state updater) and a matching onRemoveNestedPack, or wrap the IconPackEditor call with a check on inputFieldState.nestedPacks.isEmpty() to show an alternative Add-first UI/button that calls the pack-adding logic; refer to IconPackEditor, AddPackButton, onAddNestedPack, onRemoveNestedPack and inputFieldState.nestedPacks to locate where to add the guard/implementation.
56-56: Reusing"iconpack.newpack.creation.continue"string key in the Material pack flow — consider a dedicated key.The button label is borrowed from the New Pack namespace. As long as the text is identical, this works — but it creates a hidden coupling: a future New Pack wording change would silently affect Material Pack UI.
🤖 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/material/ui/MaterialPackCreation.kt` at line 56, The MaterialPackCreation composable currently reuses the "iconpack.newpack.creation.continue" string key which creates a hidden coupling; replace that key with a dedicated one (e.g., "materialpack.creation.continue") and update the strings resource file(s) accordingly, then change the Text(...) call in MaterialPackCreation to use the new key so the Material pack flow has its own independent localization entry.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackViewModel.kt (1)
83-101:updateDestinationPathandsaveDestinationare near-identical toNewPackViewModel— consider extracting shared logic.Both ViewModels implement structurally identical functions (same
PackageExtractorcall, sameDirectoryStateconstruction, sameinMemorySettings.updatepattern). The only difference isNewPackViewModel.saveDestinationalso setsflatPackage = false. This duplication will need to be kept in sync as the flows evolve.🤖 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/material/MaterialPackViewModel.kt` around lines 83 - 101, Extract the duplicated logic into a shared helper: create a function (e.g., buildDirectoryState(path: Path): DirectoryState) that uses PackageExtractor.getFrom(path.invariantSeparatorsPathString).orEmpty() and path.absolutePathString(), then have updateDestinationPath use that to set ChooseDestinationDirectoryState; likewise extract a saveDestinationToSettings(directoryState: DirectoryState, setFlatPackage: Boolean = false) that calls inMemorySettings.update { iconPackDestination = directoryState.iconPackDestination; if (setFlatPackage) flatPackage = false }, and replace both updateDestinationPath and saveDestination to call these helpers (ensure NewPackViewModel.saveDestination passes setFlatPackage = true to preserve its difference); reference functions: updateDestinationPath, saveDestination, NewPackViewModel.saveDestination, PackageExtractor, DirectoryState, ChooseDestinationDirectoryState, inMemorySettings.update, currentState.safeAs.
🤖 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/jewel/button/OutlineIconButton.kt`:
- Around line 53-56: Replace the disabled icon tint selection that currently
reads the button border color and unsafe-casts it with a semantic content color:
in OutlineIconButton (the tint variable/when expression that references
style.colors.borderDisabled and safeAs<SolidColor>()), use
style.colors.contentDisabled directly (with a fallback to Color.Unspecified)
when enabled is false so the icon uses the proper disabled foreground color and
you avoid brittle Brush casting.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackViewModel.kt`:
- Line 45: The ViewModel's initial _state is being set to a blank
ChooseDestinationDirectoryState(), which regresses restoration of persisted
destination data; change the initialization of _state
(MutableStateFlow<NewPackModeState>) to use the persisted settings by calling
inMemorySettings.current.toChooseDirectoryState() so the initial
NewPackModeState is populated with saved iconPackDestination, predictedPackage,
and nextAvailable values instead of empty defaults.
---
Outside diff comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/IconPackEditor.kt`:
- Around line 76-90: The branch that renders AddPackButton when
inputFieldState.nestedPacks.isEmpty() should also respect
inputFieldState.allowEditing; change the empty-branch to only render
AddPackButton when inputFieldState.allowEditing is true (same gating used when
nestedPacks is non-empty) so AddPackButton is not shown or clickable when
editing is disallowed, keeping behavior consistent between the empty and
non-empty branches; update the conditional around AddPackButton (referencing
inputFieldState, allowEditing, AddPackButton, onAddNestedPack, and NestedPacks)
accordingly.
---
Duplicate comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/tooling/PreviewTheme.kt`:
- Around line 36-37: Both assignments are side effects executed on every
recomposition: wrap GlobalPreviewState.isPreview = true and DI.initWith(project)
inside a DisposableEffect keyed to project (or Unit) within the composable so
they run once when entered and are cleaned up on disposal; set
GlobalPreviewState.isPreview = true in the effect and reset it to false in
DisposableEffect.onDispose, and call DI.initWith(project) in the effect and
perform any necessary DI cleanup in onDispose (refer to
GlobalPreviewState.isPreview and DI.initWith(project) to locate code).
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/existingpack/ExistingPackViewModel.kt`:
- Around line 120-125: The call in ExistingPackViewModel to
IconPackWriter.savePack inside the withContext(Dispatchers.IO) block relies on
the default writeToFile value (true); make this explicit by adding writeToFile =
false to the savePack call (matching MaterialPackViewModel and
NewPackViewModel), so update the IconPackWriter.savePack invocation in
ExistingPackViewModel to pass writeToFile = false.
---
Nitpick comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/existingpack/ExistingPackViewModel.kt`:
- Around line 49-50: Replace the MutableSharedFlow-based event bus in
ExistingPackViewModel with a rendezvous Channel to match siblings: change the
backing _events from MutableSharedFlow<ExistingPackEvent>() to a
Channel<ExistingPackEvent>(Channel.RENDEZVOUS) (or simply Channel()), expose
events as _events.receiveAsFlow(), and update all places that call
_events.emit(...) to use _events.send(...) so sends suspend until a collector is
ready; refer to symbols ExistingPackViewModel, _events, events, and
ExistingPackEvent when making the edits.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialInputHandler.kt`:
- Around line 26-32: Replace the numeric string IDs in the nestedPacks list with
semantic IDs that match the icon set names; update the NestedPack entries inside
nestedPacks (the list containing NestedPack(...) with inputFieldState =
InputState(...)) so each NestedPack.id uses the corresponding text value
("Filled", "Outlined", "Rounded", "TwoTone", "Sharp") instead of "0".."4" to
make IDs self-descriptive and match InputState.text.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackViewModel.kt`:
- Around line 83-101: Extract the duplicated logic into a shared helper: create
a function (e.g., buildDirectoryState(path: Path): DirectoryState) that uses
PackageExtractor.getFrom(path.invariantSeparatorsPathString).orEmpty() and
path.absolutePathString(), then have updateDestinationPath use that to set
ChooseDestinationDirectoryState; likewise extract a
saveDestinationToSettings(directoryState: DirectoryState, setFlatPackage:
Boolean = false) that calls inMemorySettings.update { iconPackDestination =
directoryState.iconPackDestination; if (setFlatPackage) flatPackage = false },
and replace both updateDestinationPath and saveDestination to call these helpers
(ensure NewPackViewModel.saveDestination passes setFlatPackage = true to
preserve its difference); reference functions: updateDestinationPath,
saveDestination, NewPackViewModel.saveDestination, PackageExtractor,
DirectoryState, ChooseDestinationDirectoryState, inMemorySettings.update,
currentState.safeAs.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/ui/MaterialPackCreation.kt`:
- Around line 44-46: The IconPackEditor is being passed no-op lambdas for
onAddNestedPack/onRemoveNestedPack which leads to a dead-end when
inputFieldState.nestedPacks.isEmpty(); update MaterialPackCreation so it
guards/handles empty nestedPacks by providing a real handler or conditional UI:
either supply a concrete onAddNestedPack implementation that inserts a default
nested pack into inputFieldState.nestedPacks (or calls the existing state
updater) and a matching onRemoveNestedPack, or wrap the IconPackEditor call with
a check on inputFieldState.nestedPacks.isEmpty() to show an alternative
Add-first UI/button that calls the pack-adding logic; refer to IconPackEditor,
AddPackButton, onAddNestedPack, onRemoveNestedPack and
inputFieldState.nestedPacks to locate where to add the guard/implementation.
- Line 56: The MaterialPackCreation composable currently reuses the
"iconpack.newpack.creation.continue" string key which creates a hidden coupling;
replace that key with a dedicated one (e.g., "materialpack.creation.continue")
and update the strings resource file(s) accordingly, then change the Text(...)
call in MaterialPackCreation to use the new key so the Material pack flow has
its own independent localization entry.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackScreen.kt`:
- Around line 134-141: The DirectoryState literal is duplicated in the initial
state and the onBack handler; extract it to a shared immutable preview constant
(e.g., PREVIEW_DIRECTORY_STATE or previewDirectoryState) and use that constant
when constructing ChooseDestinationDirectoryState in rememberMutableState and
inside the onBack handler; update references to
DirectoryState(iconPackDestination = "path/to/import", predictedPackage =
"com.example.iconpack", nextAvailable = true) so both NewPackModeState creation
and the onBack use the single constant (refer to rememberMutableState,
ChooseDestinationDirectoryState, DirectoryState, and the onBack handler).
| val tint = when { | ||
| enabled -> Color.Unspecified | ||
| else -> style.colors.borderDisabled.safeAs<SolidColor>()?.value ?: Color.Unspecified | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the ButtonColors definition in Jewel to verify the fields
fd -e kt -e kts | xargs rg -l "class ButtonColors|interface ButtonColors" | head -5Repository: ComposeGears/Valkyrie
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for ButtonColors definition and its properties
rg "data class ButtonColors|class ButtonColors|interface ButtonColors" -A 20 --type kotlinRepository: ComposeGears/Valkyrie
Length of output: 47
🏁 Script executed:
#!/bin/bash
# More targeted search for ButtonColors in jewel package
find . -path "*/jewel/*" -name "*.kt" | xargs rg -l "ButtonColors" | head -10Repository: ComposeGears/Valkyrie
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the file's imports and look for ButtonColors source
head -30 tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/button/OutlineIconButton.ktRepository: ComposeGears/Valkyrie
Length of output: 1378
🏁 Script executed:
#!/bin/bash
# Search for build.gradle or pom.xml to see if jewel is a dependency
find . -name "build.gradle*" -o -name "pom.xml" | xargs grep -l "jewel" 2>/dev/null | head -5Repository: ComposeGears/Valkyrie
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look for any ButtonColors usage in the codebase to understand its structure
rg "ButtonColors" -A 3 -B 3 --type kotlin | head -100Repository: ComposeGears/Valkyrie
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Read the full OutlineIconButton.kt file
cat -n tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/button/OutlineIconButton.ktRepository: ComposeGears/Valkyrie
Length of output: 3659
🏁 Script executed:
#!/bin/bash
# Search for contentDisabled usage in the entire codebase to see if it's used elsewhere
rg "contentDisabled" --type kotlinRepository: ComposeGears/Valkyrie
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for other button files that might use style.colors to understand the pattern
find . -path "*/jewel/button/*.kt" -type f -exec basename {} \; | sort | uniqRepository: ComposeGears/Valkyrie
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Look for other uses of borderDisabled in the codebase
rg "borderDisabled" --type kotlin -B 2 -A 2Repository: ComposeGears/Valkyrie
Length of output: 752
🏁 Script executed:
#!/bin/bash
# Check how other button files handle disabled/tint logic
cat -n tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/button/TooltipIconButton.ktRepository: ComposeGears/Valkyrie
Length of output: 2457
🏁 Script executed:
#!/bin/bash
# Search for all uses of style.colors in the codebase to understand the pattern
rg "style\.colors\." --type kotlin -B 2 -A 2Repository: ComposeGears/Valkyrie
Length of output: 752
🏁 Script executed:
#!/bin/bash
# Search for how colors are accessed in Jewel-based components across the codebase
rg "\.colors\." --type kotlin | grep -E "(button|Button|style\.colors)" | head -20Repository: ComposeGears/Valkyrie
Length of output: 261
🌐 Web query:
Jewel ButtonColors contentDisabled API documentation
💡 Result:
In JetBrains Jewel (Compose for Desktop), ButtonColors is part of the org.jetbrains.jewel:jewel-ui artifact, and contentDisabled is the color used for a button’s content (text/icon) when the button is disabled.
How to find the API docs:
- Online Javadoc (latest): open the
jewel-uidocs on javadoc.io and search forButtonColors→contentDisabled. The latest published version shown there is 0.33.0-253.29795. [1] - Downloadable Javadoc JAR: Maven Central publishes a
*-javadoc.jarfor that same version (useful if you want to browse docs offline or attach them in IntelliJ). [2] - Source location: the old
JetBrains/jewelrepo was archived and development moved intoJetBrains/intellij-communityunderplatform/jewel. [3]
Sources: [1] [2] [3]
🌐 Web query:
Jewel UI button colors borderDisabled contentDisabled property
💡 Result:
In JetBrains Jewel UI, the ButtonColors API exposes:
borderDisabled: the border color used when the button is disabled (enabled = false). Disabled button styling typically has a dedicated border.disabled token/key in JetBrains UI theming systems. [1]contentDisabled: the content (text/icon) color used when the button is disabled (enabled = false). Disabled button styling typically has a dedicated text.disabled token/key. [1]
You normally don’t set these as separate parameters on Button itself—rather, you provide a ButtonColors instance (via the button’s colors = ...) where borderDisabled / contentDisabled are overridden, while other states keep their defaults.
Source(s): JetBrains theme color keys reference showing distinct button.*.border.disabled and button.*.text.disabled keys for disabled button border/text colors. [1]
Use contentDisabled instead of borderDisabled for icon tinting.
borderDisabled is the button's border stroke color when disabled, not its content (icon/text) color. Jewel's ButtonColors exposes a dedicated contentDisabled property for disabled foreground colors — this is semantically correct and simpler than casting:
✂️ Proposed change
- val tint = when {
- enabled -> Color.Unspecified
- else -> style.colors.borderDisabled.safeAs<SolidColor>()?.value ?: Color.Unspecified
- }
+ val tint = if (enabled) Color.Unspecified else style.colors.contentDisabledThe current code's safeAs<SolidColor>()?.value cast and fallback also mean that if borderDisabled is ever a non-solid Brush, the disabled icon silently renders without tint — visually indistinguishable from the enabled state.
🤖 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/button/OutlineIconButton.kt`
around lines 53 - 56, Replace the disabled icon tint selection that currently
reads the button border color and unsafe-casts it with a semantic content color:
in OutlineIconButton (the tint variable/when expression that references
style.colors.borderDisabled and safeAs<SolidColor>()), use
style.colors.contentDisabled directly (with a fallback to Color.Unspecified)
when enabled is false so the icon uses the proper disabled foreground color and
you avoid brittle Brush casting.
| private val currentState: NewPackModeState | ||
| get() = state.value | ||
|
|
||
| private val _state = MutableStateFlow<NewPackModeState>(ChooseDestinationDirectoryState()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for ChooseDestinationDirectoryState and DirectoryState definitions
rg -n "data class ChooseDestinationDirectoryState|class ChooseDestinationDirectoryState|object ChooseDestinationDirectoryState" --type kotlin -A 10Repository: ComposeGears/Valkyrie
Length of output: 3116
🏁 Script executed:
rg -n "data class DirectoryState|class DirectoryState" --type kotlin -A 8Repository: ComposeGears/Valkyrie
Length of output: 829
🏁 Script executed:
# Also check if inMemorySettings is still being used in NewPackViewModel
rg -n "inMemorySettings" tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackViewModel.ktRepository: ComposeGears/Valkyrie
Length of output: 500
Regression confirmed: previously-saved destination no longer restored on ViewModel init.
The old initialization used inMemorySettings.current.toChooseDirectoryState() to pre-populate state from persisted settings. The new code initializes with ChooseDestinationDirectoryState(), which defaults to an empty DirectoryState (all fields blank). This causes previously saved iconPackDestination, predictedPackage, and nextAvailable values to be lost when the ViewModel is created, forcing returning users to re-enter their destination directory.
Since inMemorySettings is already injected and actively used elsewhere in the ViewModel (lines 35, 37, 61, 99, 121-122, 134), restore the initialization to use inMemorySettings.current.toChooseDirectoryState() to re-populate the state with saved values.
🤖 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/newpack/NewPackViewModel.kt`
at line 45, The ViewModel's initial _state is being set to a blank
ChooseDestinationDirectoryState(), which regresses restoration of persisted
destination data; change the initialization of _state
(MutableStateFlow<NewPackModeState>) to use the persisted settings by calling
inMemorySettings.current.toChooseDirectoryState() so the initial
NewPackModeState is populated with saved iconPackDestination, predictedPackage,
and nextAvailable values instead of empty defaults.
There was a problem hiding this comment.
it's new logic, i don't want old values
There was a problem hiding this comment.
@egorikftp, understood! Thank you for clarifying that this is intentional new logic. The fresh start with empty defaults for the new pack flow makes sense.
✏️ Learnings added
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 874
File: tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackViewModel.kt:45-45
Timestamp: 2026-02-18T20:05:17.471Z
Learning: In NewPackViewModel (tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackViewModel.kt), the initialization of _state with ChooseDestinationDirectoryState() using empty defaults is intentional new logic. The ViewModel should NOT restore previously-saved destination values from inMemorySettings on init; users should start with a clean state each time.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 866
File: tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/tools/svgxml/conversion/SvgXmlConversionScreen.kt:147-147
Timestamp: 2026-02-17T19:35:54.190Z
Learning: In SvgXmlConversionScreen.kt and similar editor screens using IntellijEditorTextField, the pattern `val code by rememberMutableState { state.xmlCode.value }` is intentional. The editor is initialized once with the ViewModel's value, then maintains its own internal Document state. User edits flow from editor → ViewModel via onChange callbacks (e.g., OnCodeChange action). This is the correct pattern for text editors where the user is editing content—the editor becomes the source of truth after initialization, not the ViewModel.
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/IconPackModeScreen.kt (1)
87-87: Optional: consider a more semantically appropriate icon for Material pack.
AllIconsKeys.FileTypes.SourceMapis the IDE's icon for JavaScript/TypeScript source-map files, which is thematically unrelated to Material Icons. A few alternatives that carry a "design/components" connotation:AllIconsKeys.Toolwindows.ToolWindowComponents,AllIconsKeys.Actions.Colors, or a custom icon. This has no functional impact.🤖 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/IconPackModeScreen.kt` at line 87, The icon used for the Material pack is currently AllIconsKeys.FileTypes.SourceMap which is semantically for source maps; update the icon reference in IconPackModeScreen (the key = AllIconsKeys.FileTypes.SourceMap usage) to a more appropriate icon such as AllIconsKeys.Toolwindows.ToolWindowComponents or AllIconsKeys.Actions.Colors, or swap in a custom Material icon resource; ensure you replace the referenced symbol only (key assignment) so the rest of IconPackModeScreen remains unchanged.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/IconPackDirectoryPicker.kt (1)
74-78: Refactor string keys to reflect generic usage, not NewPack-specific scope.The component is used by both
NewPackScreenandMaterialPackScreen, but all string references (lines 77, 90, 96, 107) use theiconpack.newpacknamespace prefix, which semantically indicates they are NewPack-specific. While the actual text content ("Drag & Drop destination folder", "Destination path", etc.) is generic enough to work for both flows, the string key namespace should be refactored toiconpack.commonto align with the component's placement in the common package.Consider moving the string keys to a generic namespace or parameterizing them so the string scope matches the component's intent.
🤖 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/common/IconPackDirectoryPicker.kt` around lines 74 - 78, The string keys used by IconPackDirectoryPicker (the stringResource calls in this composable that currently use the iconpack.newpack.* namespace) are NewPack-specific; refactor them to a generic namespace (e.g., iconpack.common.choose.directory.dnd, iconpack.common.destination.path, etc.) or accept string resource keys/strings as parameters to IconPackDirectoryPicker so the component is truly common; update the stringResource invocations in IconPackDirectoryPicker to use the new iconpack.common keys (or the passed-in parameters) and add corresponding entries to the resources.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/IconPackEditor.kt (1)
94-140: Clarify the semantic scope ofallowEditingparameter.The
allowEditingflag (line 135) only gates the "Add nested pack" button, but delete buttons for individual packs remain controlled by per-packinputFieldState.enabledflags. If this layered control is intentional, consider renamingallowEditingtoallowAddingPacksfor clarity, since it doesn't prevent deletion of existing packs.🤖 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/common/IconPackEditor.kt` around lines 94 - 140, The parameter allowEditing on the NestedPacks composable only controls showing the AddPackButton while deletion remains governed by each nestedPack.inputFieldState.enabled; rename allowEditing to allowAddingPacks (update the NestedPacks signature and all call sites) and update any related KDoc/comments to reflect the narrower semantics, or alternatively change behavior to gate deletes via allowAddingPacks && inputFieldState.enabled if you intend a single editing flag; adjust usages of onRemove, AddPackButton, and any docs to match the new name/behavior.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/ui/MaterialPackCreation.kt (1)
44-45: No-op lambdas for nested pack editing are intentional but could benefit from a brief comment.Since the material pack flow uses pre-defined, non-editable nested packs (
allowEditing = false), the empty lambdas are correct. A brief inline comment would clarify intent for future readers.🤖 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/material/ui/MaterialPackCreation.kt` around lines 44 - 45, Add a short inline comment beside the empty lambdas onAddNestedPack and onRemoveNestedPack in MaterialPackCreation.kt explaining they are intentionally no-ops because nested packs are pre-defined and editing is disabled (allowEditing = false); reference the onAddNestedPack and onRemoveNestedPack parameters to locate the exact spot and keep the comment concise so future readers understand this is intentional, not an omission.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackViewModel.kt (1)
41-42: Channel pattern is consistent codebase-wide; buffering not essential but could be considered.
Channel<MaterialPackEvent>()uses RENDEZVOUS capacity (default), matching the pattern acrossNewPackViewModel,SvgXmlViewModel, and other ViewModels. All send events from active coroutine contexts (user-triggered actions withinviewModelScope.launchblocks), so the UI collector is ready whensend()is called. Buffering withChannel(capacity = 1)is optional—the RENDEZVOUS pattern is intentional for one-shot event delivery and works reliably in this context.🤖 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/material/MaterialPackViewModel.kt` around lines 41 - 42, The Channel in MaterialPackViewModel using the default RENDEZVOUS capacity is intentional and consistent with other view models; update the code by adding a brief comment near the _events/ events declarations (referencing _events: Channel<MaterialPackEvent>(), events = _events.receiveAsFlow(), and the MaterialPackViewModel) stating that RENDEZVOUS is chosen intentionally for one-shot event delivery (matching NewPackViewModel, SvgXmlViewModel) and that buffering is optional because sends occur from viewModelScope coroutines where the UI collector is expected to be ready.
🤖 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/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/button/OutlineIconButton.kt`:
- Around line 53-56: The disabled icon tint logic in OutlineIconButton.kt
currently reads style.colors.borderDisabled and casts to SolidColor, which is
wrong and brittle; change the tint computation to use
style.colors.contentDisabled (no cast to SolidColor and no fallback to
Color.Unspecified) so the icon uses the button's disabled foreground color;
after updating the tint expression, remove any now-unused SolidColor and safeAs
imports.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/tooling/PreviewTheme.kt`:
- Around line 36-37: Move the side-effectful calls out of the composable body
and into a DisposableEffect keyed appropriately: set
GlobalPreviewState.isPreview = true inside a DisposableEffect and restore it to
false in onDispose, and call DI.initWith(project) once inside the same (or a
separate) DisposableEffect so it runs only on composition entry and can be
cleaned up on dispose; update the PreviewTheme composable to wrap these
assignments in DisposableEffect blocks referencing the project (or Unit if
project is stable) and perform any necessary DI shutdown/cleanup in onDispose.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackViewModel.kt`:
- Around line 109-125: saveIconPack in MaterialPackViewModel is correct to call
IconPackWriter.savePack(writeToFile = false) and set
inMemorySettings.flatPackage = true; ensure this explicit behavior is preserved
here and make the other view models symmetric by making their intent explicit:
in NewPackViewModel and ExistingPackViewModel call inMemorySettings.update {
flatPackage = false } before invoking IconPackWriter.savePack and pass
writeToFile = true explicitly to savePack, so the difference between material
packs and regular packs is clear and not implicit.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackViewModel.kt`:
- Around line 45-46: The initial _state is created with a blank
ChooseDestinationDirectoryState() which ignores persisted destination; update
the NewPackViewModel initialization so _state =
MutableStateFlow<NewPackModeState>(...) is seeded from inMemorySettings.current
(use its iconPackDestination and any derived predictedPackage/nextAvailable)
instead of empty defaults, e.g. construct a ChooseDestinationDirectoryState
populated from inMemorySettings.current values so returning users see their
saved output folder and derived fields when the ViewModel is created.
---
Nitpick comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/IconPackDirectoryPicker.kt`:
- Around line 74-78: The string keys used by IconPackDirectoryPicker (the
stringResource calls in this composable that currently use the
iconpack.newpack.* namespace) are NewPack-specific; refactor them to a generic
namespace (e.g., iconpack.common.choose.directory.dnd,
iconpack.common.destination.path, etc.) or accept string resource keys/strings
as parameters to IconPackDirectoryPicker so the component is truly common;
update the stringResource invocations in IconPackDirectoryPicker to use the new
iconpack.common keys (or the passed-in parameters) and add corresponding entries
to the resources.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/IconPackEditor.kt`:
- Around line 94-140: The parameter allowEditing on the NestedPacks composable
only controls showing the AddPackButton while deletion remains governed by each
nestedPack.inputFieldState.enabled; rename allowEditing to allowAddingPacks
(update the NestedPacks signature and all call sites) and update any related
KDoc/comments to reflect the narrower semantics, or alternatively change
behavior to gate deletes via allowAddingPacks && inputFieldState.enabled if you
intend a single editing flag; adjust usages of onRemove, AddPackButton, and any
docs to match the new name/behavior.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/IconPackModeScreen.kt`:
- Line 87: The icon used for the Material pack is currently
AllIconsKeys.FileTypes.SourceMap which is semantically for source maps; update
the icon reference in IconPackModeScreen (the key =
AllIconsKeys.FileTypes.SourceMap usage) to a more appropriate icon such as
AllIconsKeys.Toolwindows.ToolWindowComponents or AllIconsKeys.Actions.Colors, or
swap in a custom Material icon resource; ensure you replace the referenced
symbol only (key assignment) so the rest of IconPackModeScreen remains
unchanged.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackViewModel.kt`:
- Around line 41-42: The Channel in MaterialPackViewModel using the default
RENDEZVOUS capacity is intentional and consistent with other view models; update
the code by adding a brief comment near the _events/ events declarations
(referencing _events: Channel<MaterialPackEvent>(), events =
_events.receiveAsFlow(), and the MaterialPackViewModel) stating that RENDEZVOUS
is chosen intentionally for one-shot event delivery (matching NewPackViewModel,
SvgXmlViewModel) and that buffering is optional because sends occur from
viewModelScope coroutines where the UI collector is expected to be ready.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/ui/MaterialPackCreation.kt`:
- Around line 44-45: Add a short inline comment beside the empty lambdas
onAddNestedPack and onRemoveNestedPack in MaterialPackCreation.kt explaining
they are intentionally no-ops because nested packs are pre-defined and editing
is disabled (allowEditing = false); reference the onAddNestedPack and
onRemoveNestedPack parameters to locate the exact spot and keep the comment
concise so future readers understand this is intentional, not an omission.
1 participant
Screen.Recording.2026-02-18.at.21.49.46.mov
📝 Changelog
If this PR introduces user-facing changes, please update the relevant Unreleased section in changelogs: