Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA Material Pack flow was added: MaterialPackScreen, MaterialPackViewModel, MaterialPackAction/Event/State, MaterialPackCreation UI and MaterialInputHandler. Directory selection was refactored: new DirectoryState and IconPackDirectoryPicker replace the previous chooser. PackEditState and the ExpandedGroup composable were removed, and the persistent/in-memory useMaterialPack setting was deleted. InputFieldState changed (allowEditing). Multiple UI model types were moved from ui.model to model packages. New IconPackScreenScaffold and several input handlers/viewmodels were introduced or reorganized. DI preview initialization was adjusted or removed in some previews. 🚥 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().
egorikftp
force-pushed
the
feature/material-pack
branch
2 times, most recently
from
f56af14 to
88d872c
Compare
Member
Author
|
@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).
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.
egorikftp
force-pushed
the
feature/material-pack
branch
from
88d872c to
d60df29
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackScreen.kt (1)
64-98: Consider extracting the shared screen structure betweenNewPackContentandMaterialPackContent.Both
MaterialPackContentandNewPackContent(inNewPackScreen.kt) share a near-identical structural pattern:Column { Toolbar { BackAction; Title } VerticallyScrollableContainer { Column(centered) { when(state) { DirectoryPicker; Creation } } } }. The main differences are the title string, toolbar extras, and the specific creation composable. A shared scaffold composable could reduce this duplication.🤖 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 64 - 98, Extract the repeated layout in MaterialPackContent and NewPackContent into a shared composable (e.g., PackScreenScaffold) that accepts parameters for title (stringResource id), optional toolbar extras (slot lambda), and a body lambda that receives the screen state and callbacks; update MaterialPackContent to call PackScreenScaffold supplying the title stringResource("iconpack.material.title"), any toolbar extras, and a body that switches on MaterialPackState to call IconPackDirectoryPicker and MaterialPackCreation with the same callbacks (onAction, onValueChange), and do the analogous change in NewPackContent to reuse the same scaffold but pass its title and NewPackCreation composable – keep existing symbols IconPackDirectoryPicker, MaterialPackCreation, NewPackContent, MaterialPackContent and the action/callback signatures intact.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/existingpack/model/ExistingPackModeState.kt (1)
9-16: Consistent with the broader state model migration.The replacement of
PackEditStatewithInputFieldStateand the default construction pattern align with the same approach used inNewPackModeState.PickedStateandExistingPackInputHandler.One observation: the default
InputFieldState(iconPackName = InputState(), packageName = InputState(), nestedPacks = emptyList())construction is repeated verbatim in at least three locations (NewPackModeState,ExistingPackModeState,ExistingPackInputHandler). Consider extracting anInputFieldState.Emptycompanion constant to reduce duplication.🤖 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/model/ExistingPackModeState.kt` around lines 9 - 16, Introduce a shared constant to avoid repeated construction of the same empty InputFieldState: add a companion object on InputFieldState with val Empty = InputFieldState(iconPackName = InputState(), packageName = InputState(), nestedPacks = emptyList()), then replace the inline constructions in ExistingPackEditState (ExistingPackEditState.inputFieldState), NewPackModeState.PickedState, and ExistingPackInputHandler with InputFieldState.Empty so they all reference the single canonical empty instance.
🤖 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/ui/screen/mode/iconpack/existingpack/ExistingPackViewModel.kt`:
- Around line 111-128: The IconPackWriter.savePack call in
ExistingPackViewModel.saveIconPack relies on the default writeToFile value; make
this explicit to avoid divergence with MaterialPackViewModel. Update the call in
saveIconPack to pass writeToFile = true (or the appropriate boolean to match
intended behavior) so the parameter is explicit—refer to the
IconPackWriter.savePack invocation in ExistingPackViewModel.saveIconPack and the
analogous explicit writeToFile = false usage in MaterialPackViewModel to align
behavior.
---
Nitpick comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/existingpack/model/ExistingPackModeState.kt`:
- Around line 9-16: Introduce a shared constant to avoid repeated construction
of the same empty InputFieldState: add a companion object on InputFieldState
with val Empty = InputFieldState(iconPackName = InputState(), packageName =
InputState(), nestedPacks = emptyList()), then replace the inline constructions
in ExistingPackEditState (ExistingPackEditState.inputFieldState),
NewPackModeState.PickedState, and ExistingPackInputHandler with
InputFieldState.Empty so they all reference the single canonical empty instance.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackScreen.kt`:
- Around line 64-98: Extract the repeated layout in MaterialPackContent and
NewPackContent into a shared composable (e.g., PackScreenScaffold) that accepts
parameters for title (stringResource id), optional toolbar extras (slot lambda),
and a body lambda that receives the screen state and callbacks; update
MaterialPackContent to call PackScreenScaffold supplying the title
stringResource("iconpack.material.title"), any toolbar extras, and a body that
switches on MaterialPackState to call IconPackDirectoryPicker and
MaterialPackCreation with the same callbacks (onAction, onValueChange), and do
the analogous change in NewPackContent to reuse the same scaffold but pass its
title and NewPackCreation composable – keep existing symbols
IconPackDirectoryPicker, MaterialPackCreation, NewPackContent,
MaterialPackContent and the action/callback signatures intact.
1437862 to
f5a5721
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/jewel/textfield/ConfirmTextField.kt (1)
67-74:⚠️ Potential issue | 🟡 MinorEnter key may double-invoke
onValueChangevia the new blur path.
focusManager.clearFocus()is called beforeonValueChangein the Enter branch. Because Compose defers recomposition until after the current event handler returns,currentText(viarememberUpdatedState) still holds the old value when the blur callback fires synchronously insideclearFocus(). The blur block therefore seesnewText != currentTextand callsonValueChange— then the Enter branch calls it a second time with the same value.While calling
onValueChangetwice with the same string is idempotent in most cases, it's an unintended side-effect of the new blur-commit logic. The cleanest fix is to guard the blur commit with a flag set beforeclearFocus()is called from any explicit-commit path.♻️ Suggested fix using a commit-in-progress guard
+ var committingExplicitly by rememberMutableState { false } TextField( modifier = modifier .heightIn(min = 32.dp) .onFocusChanged { if (focused && !it.isFocused && !isError) { + if (!committingExplicitly) { val newText = state.text.toString() if (newText != currentText) { onValueChange(newText) } + } } focused = it.isFocused } .onKeyEvent { when (it.key) { Key.Enter -> { if (!isError) { + committingExplicitly = true focusManager.clearFocus() onValueChange(state.text.toString()) + committingExplicitly = false true } else { false } }Apply the same guard to the checkmark button's
onClickfor consistency.🤖 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 67 - 74, The Enter key handler in ConfirmTextField (Key.Enter branch) calls focusManager.clearFocus() before invoking onValueChange, causing the blur callback (which compares newText against currentText from rememberUpdatedState) to also call onValueChange and double-invoke it; add a commit-in-progress boolean flag (e.g., isCommitting) stored in the component scope, set it true immediately before calling focusManager.clearFocus() in the Key.Enter path and in the checkmark button's onClick, then have the blur callback check this flag and skip its onValueChange commit when isCommitting is true (reset the flag after committing) so explicit-commit paths do not trigger the blur commit.
🧹 Nitpick comments (5)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackInputHandler.kt (1)
9-34: Clean separation of the new-pack flow from the material-pack flow.The old
NewPackInputHandlerhaduseMaterialPackbranching with hardcoded Material Icons configuration — that responsibility is now properly split into a dedicatedMaterialInputHandler. This handler is focused and simple.One observation:
invalidateInputFieldState(lines 13–23) is identical toMaterialInputHandler.invalidateInputFieldState(same "preserve user's packageName, fill from destination if empty" logic). If these are expected to stay in sync, consider extracting a shared utility. Not a blocker since they may diverge independently.🤖 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/NewPackInputHandler.kt` around lines 9 - 34, The invalidateInputFieldState implementation in NewPackInputHandler duplicates the same "preserve user's packageName, fill from destination if empty" logic found in MaterialInputHandler.invalidateInputFieldState; extract that shared logic into a small utility (e.g., a function named resolvePackageNameText or resolvePackageNameState) and call it from both NewPackInputHandler.invalidateInputFieldState and MaterialInputHandler.invalidateInputFieldState so they stay in sync without duplicated code; update the companion object usage if needed to reuse PackageExtractor.getFrom(iconPackDestination) via the new utility and keep the handlers focused on their specific state differences.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackViewModel.kt (4)
102-108:saveDestination()doesn't resetflatPackage— diverges fromNewPackViewModel.
NewPackViewModel.saveDestination()explicitly setsflatPackage = falsewhen persisting the destination.MaterialPackViewModelomits this, leaving whateverflatPackagevalue a previous session wrote.saveIconPack()does correct this before writing, so the risk is limited to any code path that readsinMemorySettings.flatPackagebetweensaveDestination()andsaveIconPack(). Consider whether resettingflatPackagehere — or at least documenting the deliberate omission — is appropriate.♻️ Proposed alignment with `NewPackViewModel`
private fun saveDestination() { val destinationDirectoryState = currentState.safeAs<ChooseDestinationDirectoryState>() ?: return inMemorySettings.update { iconPackDestination = destinationDirectoryState.directoryState.iconPackDestination + flatPackage = 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/MaterialPackViewModel.kt` around lines 102 - 108, saveDestination() in MaterialPackViewModel updates iconPackDestination but doesn't reset inMemorySettings.flatPackage like NewPackViewModel.saveDestination() does, leaving stale values between saveDestination() and saveIconPack(); update MaterialPackViewModel.saveDestination() to explicitly set inMemorySettings.flatPackage = false (or document deliberate omission) so the saved destination state matches NewPackViewModel behavior and avoids reading stale flatPackage before saveIconPack(), referencing saveDestination(), saveIconPack(), inMemorySettings.flatPackage, and class MaterialPackViewModel when making the change.
90-100:updateDestinationPathis duplicated fromNewPackViewModel.The implementation (lines 90–100) is character-for-character identical to
NewPackViewModel.updateDestinationPath(lines 83–93 in the snippet). Extracting this into a shared utility (e.g., onDirectoryState's companion or aPackDestinationHelper) would eliminate the duplication.🤖 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 90 - 100, Extract the duplicate logic from MaterialPackViewModel.updateDestinationPath and NewPackViewModel.updateDestinationPath into a shared helper (either a DirectoryState companion factory or a new PackDestinationHelper function) that takes a Path and returns the ChooseDestinationDirectoryState/DirectoryState initialized with iconPackDestination, predictedPackage (using PackageExtractor.getFrom(...).orEmpty()), and nextAvailable = true; then replace both MaterialPackViewModel.updateDestinationPath and NewPackViewModel.updateDestinationPath to call that shared utility and update _state with its result.
42-43: Consider buffering the_eventschannel to avoid losingFinishSetupon slow collectors.
Channel<MaterialPackEvent>()creates an unbuffered (rendezvous) channel.Channel.RENDEZVOUShas no buffer at all;sendsuspends until a receiver invokesreceive. If the UI collector is momentarily inactive when_events.send(FinishSetup)is called (e.g., during a recomposition or brief suspension), the event will be held until the collector resumes — or dropped if the ViewModel scope is cancelled first. AChannel(Channel.BUFFERED)would eliminate this race.♻️ Use a buffered channel
-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 42 - 43, The _events Channel is currently unbuffered which can drop or suspend sends like _events.send(FinishSetup) if the UI collector is briefly inactive; change its creation from Channel<MaterialPackEvent>() to a buffered channel (e.g., Channel(Channel.BUFFERED) or Channel(capacity)) so events are queued for short-lived suspensions, keeping the existing events = _events.receiveAsFlow() and MaterialPackEvent/FinishSetup logic intact.
116-131: Add a comment explaining whywriteToFile = falsefor Material packs.The
writeToFile = falseparameter is intentional: Material Design Icons are imported from an external library dependency rather than generated. UnlikeNewPackViewModelandExistingPackViewModelwhich create/modify icon files on disk,MaterialPackViewModelonly updates configuration settings. Consider adding a brief inline comment to clarify this for future maintainers.🤖 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 116 - 131, In saveIconPack (MaterialPackViewModel) add a brief inline comment next to the IconPackWriter.savePack call explaining that writeToFile = false is intentional because Material packs use icons from an external Material Design library dependency (no icon files are generated on disk) and this view model only updates inMemorySettings/configuration rather than writing icon files; reference the saveIconPack method and the writeToFile = false flag so future maintainers understand the difference from NewPackViewModel/ExistingPackViewModel.
🤖 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/textfield/ConfirmTextField.kt`:
- Around line 51-57: In ConfirmTextField the Escape key handler currently calls
focusManager.clearFocus() before resetting the text which causes the
onFocusChanged blur branch to commit dirty state via onValueChange(state.text)
before the reset runs; modify the Escape handling code so you first call
state.setTextAndPlaceCursorAtEnd(currentText) (reset the text to currentText)
and only then call focusManager.clearFocus(), ensuring the blur callback sees
state.text == currentText and does not trigger onValueChange; reference
ConfirmTextField, state.setTextAndPlaceCursorAtEnd(...),
focusManager.clearFocus(), onValueChange(...) and the onFocusChanged blur block
when making the change.
---
Outside diff comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/textfield/ConfirmTextField.kt`:
- Around line 67-74: The Enter key handler in ConfirmTextField (Key.Enter
branch) calls focusManager.clearFocus() before invoking onValueChange, causing
the blur callback (which compares newText against currentText from
rememberUpdatedState) to also call onValueChange and double-invoke it; add a
commit-in-progress boolean flag (e.g., isCommitting) stored in the component
scope, set it true immediately before calling focusManager.clearFocus() in the
Key.Enter path and in the checkmark button's onClick, then have the blur
callback check this flag and skip its onValueChange commit when isCommitting is
true (reset the flag after committing) so explicit-commit paths do not trigger
the blur commit.
---
Duplicate comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/existingpack/ExistingPackViewModel.kt`:
- Around line 115-127: The call to IconPackWriter.savePack in
viewModelScope.launch relies on the default writeToFile behavior; make the
parameter explicit to match other callers — update the savePack(...) invocation
in ExistingPackViewModel (the call that passes inMemorySettings and
inputFieldState) to include writeToFile = true so the intent is clear and
consistent with NewPackViewModel's behavior.
---
Nitpick comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/material/MaterialPackViewModel.kt`:
- Around line 102-108: saveDestination() in MaterialPackViewModel updates
iconPackDestination but doesn't reset inMemorySettings.flatPackage like
NewPackViewModel.saveDestination() does, leaving stale values between
saveDestination() and saveIconPack(); update
MaterialPackViewModel.saveDestination() to explicitly set
inMemorySettings.flatPackage = false (or document deliberate omission) so the
saved destination state matches NewPackViewModel behavior and avoids reading
stale flatPackage before saveIconPack(), referencing saveDestination(),
saveIconPack(), inMemorySettings.flatPackage, and class MaterialPackViewModel
when making the change.
- Around line 90-100: Extract the duplicate logic from
MaterialPackViewModel.updateDestinationPath and
NewPackViewModel.updateDestinationPath into a shared helper (either a
DirectoryState companion factory or a new PackDestinationHelper function) that
takes a Path and returns the ChooseDestinationDirectoryState/DirectoryState
initialized with iconPackDestination, predictedPackage (using
PackageExtractor.getFrom(...).orEmpty()), and nextAvailable = true; then replace
both MaterialPackViewModel.updateDestinationPath and
NewPackViewModel.updateDestinationPath to call that shared utility and update
_state with its result.
- Around line 42-43: The _events Channel is currently unbuffered which can drop
or suspend sends like _events.send(FinishSetup) if the UI collector is briefly
inactive; change its creation from Channel<MaterialPackEvent>() to a buffered
channel (e.g., Channel(Channel.BUFFERED) or Channel(capacity)) so events are
queued for short-lived suspensions, keeping the existing events =
_events.receiveAsFlow() and MaterialPackEvent/FinishSetup logic intact.
- Around line 116-131: In saveIconPack (MaterialPackViewModel) add a brief
inline comment next to the IconPackWriter.savePack call explaining that
writeToFile = false is intentional because Material packs use icons from an
external Material Design library dependency (no icon files are generated on
disk) and this view model only updates inMemorySettings/configuration rather
than writing icon files; reference the saveIconPack method and the writeToFile =
false flag so future maintainers understand the difference from
NewPackViewModel/ExistingPackViewModel.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/NewPackInputHandler.kt`:
- Around line 9-34: The invalidateInputFieldState implementation in
NewPackInputHandler duplicates the same "preserve user's packageName, fill from
destination if empty" logic found in
MaterialInputHandler.invalidateInputFieldState; extract that shared logic into a
small utility (e.g., a function named resolvePackageNameText or
resolvePackageNameState) and call it from both
NewPackInputHandler.invalidateInputFieldState and
MaterialInputHandler.invalidateInputFieldState so they stay in sync without
duplicated code; update the companion object usage if needed to reuse
PackageExtractor.getFrom(iconPackDestination) via the new utility and keep the
handlers focused on their specific state differences.
f5a5721 to
1907270
Compare
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.
67bc61b9-1627-47d7-a0af-c11b7cb6a985.mov
📝 Changelog
If this PR introduces user-facing changes, please update the relevant Unreleased section in changelogs: