Skip to content

[Simple mode] Improve error handling during parsing#796

Merged
egorikftp merged 1 commit intomainfrom
feature/update-simple-mode
Dec 23, 2025
Merged

[Simple mode] Improve error handling during parsing#796
egorikftp merged 1 commit intomainfrom
feature/update-simple-mode

Conversation

@egorikftp
Copy link
Copy Markdown
Member

Screen.Recording.2025-12-22.at.21.08.44.mov

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Dec 22, 2025

Walkthrough

The pull request refactors state management for the simple conversion screen by replacing nullable states and event streams with explicit sealed interface variants. SimpleConversionState becomes a sealed interface with nested ConversionState, Error, and Loading types. The screen UI now uses when-branching to handle all state paths. parseIcon methods return Result instead of nullable types. ErrorContent component gains an optional description parameter for displaying error details. State updates occur through state assignments rather than event emissions.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description references issue #794 and includes a demonstration asset, indicating the work addresses a specific tracked issue with visual confirmation.
Title check ✅ Passed The title accurately describes the main objective of the PR—improving error handling during the parsing process in simple mode—and aligns with the substantial refactoring across multiple files.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/update-simple-mode

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/simple/conversion/SimpleConversionViewModel.kt (1)

46-76: Consider adding onFailure for settings-triggered re-parsing.

Similar to changeIconName, the settings change handler only handles onSuccess. While failures here are less likely (the source successfully parsed before), silent failures could leave users confused if something goes wrong during re-generation.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8bb9801 and f5f7122.

📒 Files selected for processing (5)
  • tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/simple/conversion/SimpleConversionScreen.kt
  • tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/simple/conversion/SimpleConversionViewModel.kt
  • tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/simple/conversion/model/ConversionState.kt
  • tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportScreenComponents.kt
  • tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/MaterialSymbolsImportScreen.kt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-07T20:07:49.753Z
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 750
File: tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt:71-85
Timestamp: 2025-12-07T20:07:49.753Z
Learning: In the Valkyrie Gradle plugin (Kotlin), the `useFlatPackage` flag in `IconPackExtension` is only applicable when nested packs are configured. For single icon packs (without nested packs), the flag is intentionally not propagated to `ImageVectorGeneratorConfig` as there is no package hierarchy to flatten.

Applied to files:

  • tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/simple/conversion/model/ConversionState.kt
🧬 Code graph analysis (3)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/MaterialSymbolsImportScreen.kt (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportScreenComponents.kt (1)
  • ErrorContent (71-122)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/simple/conversion/SimpleConversionScreen.kt (3)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/platform/CopyPasteManager.kt (1)
  • copyInClipboard (10-12)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/foundation/TopAppBar.kt (4)
  • TopAppBar (34-48)
  • BackAction (63-76)
  • AppBarTitle (50-61)
  • SettingsAction (107-120)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportScreenComponents.kt (2)
  • ErrorContent (71-122)
  • LoadingContent (46-66)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportScreenComponents.kt (1)
compose/core/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/core/layout/Spacer.kt (1)
  • VerticalSpacer (15-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build (ubuntu-latest)
🔇 Additional comments (7)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/simple/conversion/model/ConversionState.kt (1)

7-21: Well-structured sealed interface for explicit state handling.

Clean design with proper use of data class for stateful variants and data object for the stateless Loading. The @Stable annotation on the interface correctly propagates to all variants for Compose stability.

tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/MaterialSymbolsImportScreen.kt (1)

134-141: Consistent error state centering.

Wrapping ErrorContent in a centered Box aligns with the LoadingContent pattern (which also centers within fillMaxSize) and compensates for ErrorContent no longer having intrinsic full-size behavior.

tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportScreenComponents.kt (1)

71-121: Good enhancement with optional description support.

The decorative description block with the vertical line indicator provides a nice visual treatment for error details. The maxLines = 5 constraint prevents overflow while showing sufficient context.

Note that ErrorContent no longer fills the available space intrinsically. Callers requiring centered error content (like MaterialSymbolsImportScreen) now need to wrap this in a centered container, which has been done correctly in this PR.

tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/simple/conversion/SimpleConversionScreen.kt (2)

62-106: Clean exhaustive state handling with proper UX.

The when-branching ensures all state variants are handled explicitly. The error state maintains navigation capabilities (back/settings actions), which is good UX—users can navigate away or access settings even when an error occurs.


148-187: Preview correctly updated for new state type.

tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/simple/conversion/SimpleConversionViewModel.kt (2)

141-180: Clean Result-based parsing pattern.

Using runCatching to wrap the parsing logic and returning Result<IconContent> enables clean functional error handling at call sites with onSuccess/onFailure.


35-45: Correct async initialization pattern.

Starting with Loading state and asynchronously parsing in init prevents any UI flash of empty content. The state is properly updated on success or failure.

@egorikftp egorikftp changed the title [Simple mode] Handle errors during parsing [Simple mode] Improve error handling during parsing Dec 22, 2025
@egorikftp egorikftp merged commit 372c964 into main Dec 23, 2025
5 checks passed
@egorikftp egorikftp deleted the feature/update-simple-mode branch December 23, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant