[#541] Integrate the Twitter Jetpack Compose Rules Detekt plugin#556
Conversation
ryan-conway
temporarily deployed
to
template-compose
GitHub Actions
Inactive
Kover report for template-compose:🧛 Template - Compose Unit Tests Code Coverage:
|
| File | Coverage |
|---|---|
HomeScreen.kt |
67.89% |
HomeViewModel.kt |
100.00% |
Modified Files Not Found In Coverage Report:
HomeScreenTest.kt
ItemList.kt
SecondScreen.kt
ThirdScreen.kt
build.gradle.kts
build.gradle.kts
build.gradle.kts
build.gradle.kts
detekt-config.yml
detekt-config.yml
libs.versions.toml
libs.versions.toml
Codebase cunningly covered by count Shroud 🧛
Generated by 🚫 Danger
ryan-conway
requested review from
AVI5HEK,
Wadeewee,
chornerman,
doannimble,
hoangnguyen92dn,
kaungkhantsoe,
luongvo,
lydiasama,
manh-t,
minhnimble,
sleepylee,
thiennguyen0196 and
toby-thanathip
as code owners
|
Don't we need to apply these changes to sample @ryan-conway ? |
|
@ryan-conway Conflict 😶🌫️ |
|
IMO, the |
ryan-conway
force-pushed
the
feature/541-integrate-the-twitter-jetpack-compose-rules-detekt-plugin
branch
from
5662c4f to
391946a
Compare
ryan-conway
temporarily deployed
to
template-compose
GitHub Actions
Inactive
ryan-conway
had a problem deploying
to
template-compose
GitHub Actions
Error
ryan-conway
temporarily deployed
to
template-compose
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt (1)
22-28: Duplicate parametermodelwill cause compilation error.The
model: UiModel?parameter appears twice in the function signature (lines 24 and 27). This will fail to compile.Proposed fix - remove the duplicate parameter
@Composable fun ThirdScreen( model: UiModel?, viewModel: ThirdViewModel = hiltViewModel(), navigator: (destination: BaseDestination) -> Unit, - model: UiModel?, ) = BaseScreen(sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.kt (1)
24-39: Critical: Duplicate parameterid: Stringwill cause compilation error.The
id: Stringparameter is declared twice in theSecondScreenfunction signature (lines 26 and 29). This will fail to compile.🐛 Proposed fix
@Composable fun SecondScreen( - id: String, viewModel: SecondViewModel = hiltViewModel(), navigator: (destination: BaseDestination) -> Unit, id: String, ) = BaseScreen(sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.kt (1)
29-39: Critical: Duplicate parameterisResultOkwill cause compilation error.The
isResultOk: Boolean = falseparameter is declared twice in theHomeScreenfunction signature (lines 31 and 34). This will fail to compile.🐛 Proposed fix
@Composable fun HomeScreen( - isResultOk: Boolean = false, viewModel: HomeViewModel = hiltViewModel(), navigator: (destination: BaseDestination) -> Unit, isResultOk: Boolean = false, ) = BaseScreen(
🤖 Fix all issues with AI agents
In @sample-compose/gradle/libs.versions.toml:
- Line 16: Update the two dependency versions in libs.versions.toml: change the
version value for the kotlinxCollectionsImmutable key from 0.3.6 to 0.4.0, and
change the detektRules key from 0.3.3 to 0.5.3 so they use the latest stable
releases (ensure the entries named kotlinxCollectionsImmutable and detektRules
are updated accordingly).
🧹 Nitpick comments (3)
template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeViewModel.kt (1)
10-10: Consider using explicit imports instead of wildcard import.While wildcard imports work, explicit imports improve code readability and make dependencies clearer. The sample-compose module uses explicit imports for the same types.
Suggested change
-import kotlinx.collections.immutable.* +import kotlinx.collections.immutable.ImmutableList +import kotlinx.collections.immutable.persistentListOf +import kotlinx.collections.immutable.toImmutableListtemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeScreen.kt (1)
26-26: Consider using explicit imports for consistency with sample-compose.Same suggestion as for
HomeViewModel.kt- explicit imports improve clarity.Suggested change
-import kotlinx.collections.immutable.* +import kotlinx.collections.immutable.ImmutableList +import kotlinx.collections.immutable.persistentListOfsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.kt (1)
27-27: Potentially unused wildcard import.The
kotlinx.coroutines.flow.*import appears unused—flows are collected viacollectAsStateWithLifecycleandcollectAsEffectextension functions. Verify this import is necessary or remove it.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
sample-compose/app/build.gradle.ktssample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeViewModel.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/ItemList.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.ktsample-compose/build.gradle.ktssample-compose/detekt-config.ymlsample-compose/gradle/libs.versions.tomltemplate-compose/app/build.gradle.ktstemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeScreen.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeViewModel.kttemplate-compose/build.gradle.ktstemplate-compose/detekt-config.ymltemplate-compose/gradle/libs.versions.toml
🧰 Additional context used
🧬 Code graph analysis (2)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt (1)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/common/AppBar.kt (1)
AppBar(16-29)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.kt (1)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/common/AppBar.kt (1)
AppBar(16-29)
⏰ 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). (3)
- GitHub Check: Review pull request
- GitHub Check: Verify newproject script
- GitHub Check: Run Detekt and unit tests
🔇 Additional comments (20)
sample-compose/detekt-config.yml (2)
286-289: LGTM!Adding
ignoreAnnotated: [ 'Preview' ]toMagicNumberis appropriate since@Previewcomposables commonly use magic numbers for sample data and dimensions.
352-395: Compose rules configuration is correct.All compose-specific rules from the mrmans0n/compose-rules plugin are enabled. The
CompositionLocalAllowlistcorrectly includes all app-specific composition locals declared in the codebase:LocalAppColors,LocalAppDimensions,LocalAppShapes,LocalAppStyles, andLocalAppTypography.sample-compose/app/build.gradle.kts (1)
136-136: LGTM!The
kotlinx-collections-immutabledependency is correctly added to supportImmutableListusage, addressing theUnstableCollectionsrule from the compose-rules plugin.sample-compose/build.gradle.kts (1)
15-17: LGTM!Adding the compose-rules as a
detektPluginsdependency at the root level ensures the rules apply across all modules in the detekt analysis.template-compose/app/build.gradle.kts (1)
140-141: LGTM!The
kotlinx-collections-immutabledependency is correctly added to the template, ensuring generated projects have support for stable collections in Compose.template-compose/build.gradle.kts (1)
15-17: LGTM!The compose-rules plugin dependency is correctly added at the root level, consistent with the sample-compose configuration.
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/ItemList.kt (1)
11-16: LGTM!The migration to
ImmutableList<UiModel>is correctly implemented with appropriate imports and the preview is properly updated to usepersistentListOf. This aligns with the compose-rules detekt plugin requirements for stable collections.template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeViewModel.kt (1)
20-28: LGTM!The
MutableStateFlowis correctly typed withImmutableList<UiModel>, initialized withpersistentListOf(), and emissions properly usetoImmutableList()to convert the mapped results.sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt (2)
34-43: LGTM!Adding
modifier: Modifier = Modifierparameter and passing it toScaffoldfollows Compose best practices. This aligns with the compose-rules detekt plugin recommendations.
59-65: LGTM!Making
ThirdScreenPreviewprivate addresses thePreviewPublicdetekt rule correctly.template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeScreen.kt (2)
38-49: LGTM!The state binding correctly uses
ImmutableList<UiModel>andHomeScreenContentaccepts the immutable collection type. This properly addresses the unstable collections warning from the compose-rules detekt plugin.
66-74: LGTM!The preview is correctly marked as
privateand usespersistentListOffor initialization.sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeViewModel.kt (2)
13-15: Good use of explicit imports.The explicit imports for
ImmutableList,persistentListOf, andtoImmutableListare clearer than the wildcard imports used in the template-compose module.
32-43: LGTM!The
MutableStateFlowis correctly initialized withpersistentListOf()and emissions usetoImmutableList()to convert the mapped results. This implementation is consistent with the template-compose module.sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.kt (1)
41-52: LGTM on modifier addition and Scaffold refactor.The
modifierparameter with default follows Compose best practices, and the parameter ordering (required → modifier with default → lambda) adheres to theComposableParamOrderrule. The Scaffold now correctly receives the modifier.sample-compose/gradle/libs.versions.toml (1)
85-87: LGTM on detekt-compose-rules library entry.The library mapping correctly references the maintained fork at
io.nlopez.compose.ruleswith thedetektRulesversion reference.sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.kt (1)
43-43: LGTM on ImmutableList adoption.Using
ImmutableList<UiModel>for UI state andpersistentListOfin previews correctly addresses theUnstableCollectionsdetekt rule while ensuring Compose can skip recomposition when collections haven't changed.Also applies to: 94-94, 125-125
template-compose/detekt-config.yml (2)
286-289: LGTM on MagicNumber Preview exemption.Adding
ignoreAnnotated: ['Preview']is a sensible choice—preview functions often use hardcoded dimensions and colors for demonstration purposes.
352-395: LGTM on Compose rules configuration.Comprehensive coverage of Compose-specific detekt rules. The
CompositionLocalAllowlistis correctly configured with the theme's composition locals (LocalAppColors,LocalAppDimensions,LocalAppShapes,LocalAppStyles,LocalAppTypography).template-compose/gradle/libs.versions.toml (1)
16-16: LGTM on version catalog additions.The new version entries and library mappings mirror the sample-compose catalog, maintaining consistency across modules. The detekt-compose-rules and kotlinx-collections-immutable dependencies are correctly declared.
Also applies to: 24-24, 64-64, 84-86
| core = "1.15.0" | ||
| datastore = "1.1.3" | ||
| detekt = "1.21.0" | ||
| detektRules = "0.3.3" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
kotlinx-collections-immutable latest stable version
💡 Result:
Latest stable release: org.jetbrains.kotlinx:kotlinx-collections-immutable — version 0.4.0. [1][2]
References:
- Maven Central listing (artifact page shows version 0.4.0). [1]
- GitHub repository README (usage example shows 0.4.0). [2]
🌐 Web query:
detekt-compose-rules latest version
💡 Result:
Latest release: io.nlopez.compose.rules:detekt version 0.5.3 (as of January 12, 2026). [1][2]
Add to Gradle:
dependencies { detektPlugins "io.nlopez.compose.rules:detekt:0.5.3" }. [2]
Sources: [1] mvnrepository.io entry for the detekt artifact. [2] official compose-rules docs (Using with detekt).
🌐 Web query:
detektRules gradle library latest version
💡 Result:
Do you mean the Detekt Gradle plugin or a detekt "rules" library (e.g. detekt-formatting / detekt-rules-libraries)? Quick answers:
- Detekt Gradle plugin (plugin id io.gitlab.arturbosch.detekt / io.github.detekt...): latest stable 1.23.8. [1]
- detekt rules library example — dev.detekt:detekt-rules-libraries: latest published 2.0.0-alpha.1 (alpha). [2][3]
Which one do you need (and do you want stable vs. alpha)?
Sources:
[1] Gradle Plugin Portal — io.gitlab.arturbosch.detekt (1.23.8).
[2] Maven Central / MVNRepository — dev.detekt:detekt-rules-libraries (2.0.0-alpha.1).
[3] Detekt docs (rules / plugins).
Update dependency versions to latest stable releases.
The added versions are outdated. Update to:
kotlinxCollectionsImmutablefrom 0.3.6 to 0.4.0 (latest stable)detektRulesfrom 0.3.3 to 0.5.3 (latest stable for io.nlopez.compose.rules:detekt)
Also applies to: 24-24
🤖 Prompt for AI Agents
In @sample-compose/gradle/libs.versions.toml at line 16, Update the two
dependency versions in libs.versions.toml: change the version value for the
kotlinxCollectionsImmutable key from 0.3.6 to 0.4.0, and change the detektRules
key from 0.3.3 to 0.5.3 so they use the latest stable releases (ensure the
entries named kotlinxCollectionsImmutable and detektRules are updated
accordingly).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -13,13 +13,15 @@ composeNavigation = "2.5.3" | |||
| core = "1.15.0" | |||
There was a problem hiding this comment.
| A newer version of androidx.core:core-ktx than 1.15.0 is available: 1.17.0 |
| @@ -13,13 +13,15 @@ composeNavigation = "2.5.3" | |||
| core = "1.15.0" | |||
| datastore = "1.1.3" | |||
There was a problem hiding this comment.
| A newer version of androidx.datastore:datastore-preferences than 1.1.3 is available: 1.2.0 |
| datastore = "1.1.3" | ||
| detekt = "1.21.0" | ||
| detektRules = "0.3.3" | ||
| gradle = "8.8.2" |
There was a problem hiding this comment.
| A newer version of com.android.application than 8.8.2 is available: 8.13.2 |
| datastore = "1.1.3" | ||
| detekt = "1.21.0" | ||
| detektRules = "0.3.3" | ||
| gradle = "8.8.2" |
There was a problem hiding this comment.
| A newer version of com.android.library than 8.8.2 is available: 8.13.2 |
| detektRules = "0.3.3" | ||
| gradle = "8.8.2" | ||
| hilt = "2.53" | ||
| hiltNavigation = "1.2.0" |
There was a problem hiding this comment.
| A newer version of androidx.hilt:hilt-navigation-compose than 1.2.0 is available: 1.3.0 |
hoangnguyen92dn
had a problem deploying
to
template-compose
GitHub Actions
Error
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)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.kt (1)
24-38: UnusedviewModelparameter.The
viewModelparameter is injected viahiltViewModel()but never used in the function body. If this is intentional (e.g., reserved for future use), consider suppressing the warning. Otherwise, remove the unused parameter to avoid unnecessary dependency injection.Option 1: Remove if not needed
@Composable fun SecondScreen( id: String, navigator: (destination: BaseDestination) -> Unit, - viewModel: SecondViewModel = hiltViewModel(), ) = BaseScreen(Option 2: Suppress if intentional placeholder
+@Suppress("UNUSED_PARAMETER") @Composable fun SecondScreen( id: String, navigator: (destination: BaseDestination) -> Unit, viewModel: SecondViewModel = hiltViewModel(), ) = BaseScreen(
🤖 Fix all issues with AI agents
In
@sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.kt:
- Around line 36-39: The androidTest call site to HomeScreen is missing the
required isResultOk parameter; update the HomeScreen invocation in
HomeScreenTest (androidTest) to pass a boolean (e.g., isResultOk = false)
alongside viewModel and navigator so the call matches the function signature of
HomeScreen(isResultOk: Boolean, navigator: (destination: BaseDestination) ->
Unit, viewModel: HomeViewModel = hiltViewModel()).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.ktsample-compose/app/src/test/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreenTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt
⏰ 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). (3)
- GitHub Check: Review pull request
- GitHub Check: Verify newproject script
- GitHub Check: Run Detekt and unit tests
🔇 Additional comments (6)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.kt (2)
40-51: LGTM! Good adoption of Compose best practices.Adding the
modifierparameter with a default value and applying it to the rootScaffoldfollows Compose conventions. The parameter ordering (required params → optional params with defaults) is also correct.
75-84: Preview visibility is correct.Making the preview function
privatealigns with thePreviewPublicrule from the compose-rules plugin being integrated in this PR.sample-compose/app/src/test/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreenTest.kt (1)
119-123: LGTM!The test correctly passes
isResultOk = falseto match the updatedHomeScreensignature.Consider adding a test case for
isResultOk = trueto verify the toast message (R.string.message_updated) is displayed correctly.sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.kt (3)
48-48: LGTM!Using
ImmutableList<UiModel>addresses Compose stability warnings by making the collection type stable. This aligns with the Twitter Compose Rules plugin requirements.
97-103: LGTM!The
HomeScreenContentparameter type is correctly updated toImmutableList<UiModel>, consistent with the state collected from the ViewModel.
125-135: LGTM!The preview correctly uses
persistentListOf(...)to construct theImmutableList<UiModel>for the updated parameter type.
| fun HomeScreen( | ||
| viewModel: HomeViewModel = hiltViewModel(), | ||
| isResultOk: Boolean , | ||
| navigator: (destination: BaseDestination) -> Unit, | ||
| isResultOk: Boolean = false, | ||
| viewModel: HomeViewModel = hiltViewModel(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of HomeScreen to verify they pass the isResultOk parameter
# First, search for HomeScreen constructor/function calls
rg -n --type=kotlin 'HomeScreen\s*\(' -A 3 -B 1Repository: nimblehq/android-templates
Length of output: 4990
🏁 Script executed:
#!/bin/bash
# Check the complete HomeScreen call in MainNavGraph.kt
cat -n sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/MainNavGraph.kt | sed -n '24,32p'
echo "---"
# Check the complete HomeScreen call in androidTest
cat -n sample-compose/app/src/androidTest/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreenTest.kt | sed -n '84,90p'Repository: nimblehq/android-templates
Length of output: 774
Update the androidTest HomeScreenTest call site to pass the required isResultOk parameter.
The isResultOk parameter is required (no default value), and while the unit test and production navigation code have been updated, the androidTest is missing this parameter:
androidTest HomeScreenTest.kt (line 85)
HomeScreen(
viewModel = viewModel,
navigator = { destination -> expectedDestination = destination }
)Should be:
HomeScreen(
isResultOk = false, // Add this
viewModel = viewModel,
navigator = { destination -> expectedDestination = destination }
)🤖 Prompt for AI Agents
In
@sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.kt
around lines 36 - 39, The androidTest call site to HomeScreen is missing the
required isResultOk parameter; update the HomeScreen invocation in
HomeScreenTest (androidTest) to pass a boolean (e.g., isResultOk = false)
alongside viewModel and navigator so the call matches the function signature of
HomeScreen(isResultOk: Boolean, navigator: (destination: BaseDestination) ->
Unit, viewModel: HomeViewModel = hiltViewModel()).
hoangnguyen92dn
force-pushed
the
feature/541-integrate-the-twitter-jetpack-compose-rules-detekt-plugin
branch
from
c7c86a8 to
570ba90
Compare
hoangnguyen92dn
had a problem deploying
to
template-compose
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.kt (1)
24-38: UnusedviewModelparameter.The
viewModelparameter is declared and injected viahiltViewModel()but is never used in the function body. This causes unnecessary ViewModel instantiation.Either remove the unused parameter or utilize it if state/actions from the ViewModel are needed.
Proposed fix if viewModel is not needed
@Composable fun SecondScreen( id: String, navigator: (destination: BaseDestination) -> Unit, - viewModel: SecondViewModel = hiltViewModel(), ) = BaseScreen(sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.kt (1)
77-95: Toast calls during composition can cause repeated toasts on recomposition.The
showToastcalls on lines 83, 86, and 88 are executed directly during composition. This means every recomposition will trigger a new toast, which is likely unintended. Consider wrapping these inLaunchedEffectkeyed on the permission state:@OptIn(ExperimentalPermissionsApi::class) @Composable private fun CameraPermission() { val context = LocalContext.current val cameraPermissionState = rememberPermissionState(CAMERA) - if (cameraPermissionState.status.isGranted) { - context.showToast("${cameraPermissionState.permission} granted") - } else { - if (cameraPermissionState.status.shouldShowRationale) { - context.showToast("${cameraPermissionState.permission} needs rationale") - } else { - context.showToast("Request cancelled, missing permissions in manifest or denied permanently") - } - - LaunchedEffect(Unit) { - cameraPermissionState.launchPermissionRequest() - } - } + LaunchedEffect(cameraPermissionState.status) { + if (cameraPermissionState.status.isGranted) { + context.showToast("${cameraPermissionState.permission} granted") + } else { + if (cameraPermissionState.status.shouldShowRationale) { + context.showToast("${cameraPermissionState.permission} needs rationale") + } else { + context.showToast("Request cancelled, missing permissions in manifest or denied permanently") + } + cameraPermissionState.launchPermissionRequest() + } + } }Additionally, consider extracting the hardcoded strings to string resources for localization.
🧹 Nitpick comments (2)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt (1)
22-31: UnusedviewModelandnavigatorparameters.Both
viewModelandnavigatorare declared but never used withinThirdScreen. If these are intentional placeholders for the template pattern, consider adding a suppress annotation or a comment. Otherwise, remove them to avoid confusion.Option 1: Suppress if intentional placeholder
+@Suppress("UnusedParameter") @Composable fun ThirdScreen( model: UiModel?, navigator: (destination: BaseDestination) -> Unit, viewModel: ThirdViewModel = hiltViewModel(), ) = BaseScreen(Option 2: Remove if not needed
@Composable fun ThirdScreen( model: UiModel?, - navigator: (destination: BaseDestination) -> Unit, - viewModel: ThirdViewModel = hiltViewModel(), ) = BaseScreen( isDarkStatusBarIcons = true, ) { ThirdScreenContent(data = model) }sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.kt (1)
58-62: Consider usingisResultOkas the LaunchedEffect key.Using
Unitas the key means this effect runs only once on initial composition. IfisResultOkcould change during the screen's lifecycle, consider using it as the key instead:- LaunchedEffect(Unit) { + LaunchedEffect(isResultOk) { if (isResultOk) { context.showToast(context.getString(R.string.message_updated)) } }If the intent is to show the toast only once when navigating to this screen with
isResultOk = true, the current implementation is fine.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.ktsample-compose/app/src/test/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreenTest.kt
🧰 Additional context used
🧬 Code graph analysis (2)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt (2)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/base/BaseScreen.kt (1)
BaseScreen(8-21)sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/common/AppBar.kt (1)
AppBar(16-29)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.kt (1)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/common/AppBar.kt (1)
AppBar(16-29)
⏰ 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). (3)
- GitHub Check: Verify newproject script
- GitHub Check: Review pull request
- GitHub Check: Run Detekt and unit tests
🔇 Additional comments (8)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt (2)
33-56: LGTM!Good improvements following Compose best practices:
- Made
ThirdScreenContentprivate (internal implementation detail)- Added
modifierparameter with default value as the last parameter- Correctly forwarded
modifierto the rootScaffoldcomposable
58-64: LGTM!Making
@Previewfunctions private is correct per thePreviewPublicrule from the compose-rules plugin. Preview functions are only used by tooling and shouldn't be exposed as public API.sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.kt (2)
40-51: LGTM!Good additions:
- The
modifierparameter with a default value follows Compose best practices for reusable composables.- Parameter ordering is correct (required params first, then optional).
- Passing the modifier to
Scaffoldallows external customization of the content layout.
75-84: LGTM!The preview is correctly marked as
private, which addresses thePreviewPublicrule from the Twitter Compose Rules Detekt plugin.sample-compose/app/src/test/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreenTest.kt (1)
119-123: LGTM!The test correctly adapts to the new
isResultOkparameter inHomeScreen. Explicitly passingfalseis good practice for test clarity.sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.kt (3)
3-6: LGTM!The imports are well-organized and explicit. The addition of Accompanist permissions API and kotlinx.collections.immutable aligns with the PR objectives.
Also applies to: 9-11, 28-33
97-103: LGTM!Using
ImmutableList<UiModel>for theuiModelsparameter improves Compose stability and allows the compiler to skip recomposition when the list reference hasn't changed. This aligns with the compose-rules detekt plugin requirements.
125-136: LGTM!The preview correctly uses
persistentListOfto create anImmutableList, and the function is appropriately marked asprivateper the PreviewPublic compose rule.
hoangnguyen92dn
had a problem deploying
to
template-compose
GitHub Actions
Error
hoangnguyen92dn
deleted the
feature/541-integrate-the-twitter-jetpack-compose-rules-detekt-plugin
branch
10 participants
closes #541
What happened 👀
detekt-config.ymlkotlinx.collections.immutabledependency and updated UI lists to useImmutableListsample-composeInsight 📝
Collection(i.e., List, Set, etc.) as unstable, we should update all UI usages of collections to use theirImmutableXcounterpart to reduce unnecessary recomposition.ignoreAnnotated: [ 'Preview' ]to theMagicNumberrule as preview composables typically contain hard-coded values and we don't want these to be flagged 👀Proof Of Work 📹
With incorrect composable name (
homeScreenContent), public@Previewand unstable lists:With fixes applied:
Summary by CodeRabbit
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.