Skip to content

[JEWEL-920] Add Detekt Compose rules to the project #3161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rock3r
Copy link
Collaborator

@rock3r rock3r commented Aug 1, 2025

We now apply additional Compose rules to our Detekt checks. The rules are documented at https://mrmans0n.github.io/compose-rules/rules/

A couple of rules have been disabled as they do not apply to Jewel as a library. The rules JAR was added to the repository so that it can also be used by the IntelliJ Detekt plugin, and show issues in the IJ editor.

This applies trivial fixes from the rules, leaving challenging ones for a later time (suppressed and filed tech debt issues to fix them).

🚨 IMPORTANT REQUEST

I'd appreciate it if when reviewing this PR y'all did some smoke testing, too. I did it, and it's fine, but just in case...

Notable changes from applying the rules:

  • Fixed a bug in BasicLazyTree where a derived state was not remembered
  • Fixed a bug in ButtonImpl where the onStateChange, accessed in a LaunchedEffect, was not used as a key to the effect
    • This effectively was not causing issues as it’s a private impl and we control the lambdas, but was a mistake nonetheless
    • We have recently fixed a similar bug in JEWEL-868
  • Fixed the same kind of bug in ListComboBox
  • Fixed the same kind of bug in CircularProgressIndicator
  • Fixed several missing remember keys in PopupMenu's MenuContent
  • Fixed a bug in ContextMenu where the focus manager and input mode manager were not remembered
  • Deprecated several mistakenly public APIs in Menu.kt, and made them to delegate to a now-private implementation, so the public one can be safely removed in the future
  • Made sure there is no indication set by default, instead of the stock one provided by Compose
  • Adjusted the signature of many internal composables
  • Improved the SLC and tree samples in the showcase
  • Added a simpler variant to Modifier.outline()
  • Removed duplication in ComboBox implementations by making the string-based variant
    delegate to slot-based one

This also adds an artefact configuration, so that we get a JAR for our own Detekt rules plugin when the IDE is built from JPS.

Release notes

⚠️ Important Changes

  • The default Indication has been set to a no-op implementation in both standalone and bridge, instead of the previous default implementation we inherited from Compose. We handle visual states separately from the Indication API, and as such this would only cause visual issues when using certain modifiers (e.g., selectable)
  • The experimental slot-based ComboBox overload has changed in a breaking way by reordering its parameters

New features

  • Added a simpler variant to Modifier.outline()

Bug fixes

  • Fixed a bug in BasicLazyTree where the item background state was not properly remembered
  • Fixed a bug in ListComboBox where changing the itemKeys parameter value would not be picked up by the component until it exited and re-entered the composition
  • Fixed a bug in CircularProgressIndicator where changing the frameRetriever parameter value would not be picked up by the component until it exited and re-entered the composition
  • Fixed a bug where the PopupMenu was over-remembering some internal state

Deprecated API

  • Deprecated several APIs in Menu.kt that were left public by mistake so they can be made private as they should, in the future

@rock3r rock3r self-assigned this Aug 1, 2025
@rock3r rock3r added the Jewel label Aug 1, 2025
@rock3r rock3r requested review from DanielSouzaBertoldi and faogustavo and removed request for DanielSouzaBertoldi August 1, 2025 12:39
@rock3r rock3r force-pushed the sebp/JEWEL-920_add-detekt-compose-rules branch 2 times, most recently from f87690a to 4e6243c Compare August 5, 2025 09:44
@rock3r rock3r force-pushed the sebp/JEWEL-920_add-detekt-compose-rules branch 4 times, most recently from b69cce0 to 590a1b5 Compare August 7, 2025 11:25
@rock3r
Copy link
Collaborator Author

rock3r commented Aug 7, 2025

Ready to merge

We now apply additional Compose rules to our Detekt checks. The rules
are documented at https://mrmans0n.github.io/compose-rules/rules/

A couple of rules have been disabled as they do not apply to Jewel as
a library. The rules JAR was added to the repository so that it can
also be used by the IntelliJ Detekt plugin, and show issues in the IJ
editor.

This applies trivial fixes from the rules, leaving difficult ones for
a later time (suppressed and filed tech debt issues to fix them).

Notable changes from applying the rules:
 * Fixed a bug in `BasicLazyTree` where a derived state was not
   remembered.
 * Fixed a bug in `ButtonImpl` where the `onStateChange`, accessed in
   a `LaunchedEffect`, was not used as a key to the effect. We have
   recently fixed a similar bug in JEWEL-868.
 * Fixed the same kind of bug in `ListComboBox`.
 * Fixed the same kind of bug in `CircularProgressIndicator`.
 * Fixed several missing remember keys in `PopupMenu`'s `MenuContent`.
 * Fixed a bug in `ContextMenu` where the focus manager and input mode
   manager were not remembered.
 * Deprecated several mistakenly public APIs in Menu.kt, and made them
   to delegate to a now-private implementation, so the public one can
   be safely removed in the future.
 * Made sure there is no indication set by default, instead of the
   stock one provided by Compose.
 * Adjusted the signature of many internal composables.
 * Improved the SLC and tree samples in the showcase.
 * Added a simpler variant to Modifier.outline().

This also adds an artifact so that we get a JAR for our own Detekt
rules plugin when the IDE is built from JPS.
@rock3r rock3r force-pushed the sebp/JEWEL-920_add-detekt-compose-rules branch from 590a1b5 to 0058820 Compare August 9, 2025 10:09
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.

3 participants