๐ :: (#881) ์๋ฅ ์ ์ฒญ ๋ฒํผ ์๋ฆผ ๋ฌธ์ ํด๊ฒฐ #883
๐ :: (#881) ์๋ฅ ์ ์ฒญ ๋ฒํผ ์๋ฆผ ๋ฌธ์ ํด๊ฒฐ #883
Conversation
|
No actionable comments were generated in the recent review. ๐ โน๏ธ Recent review infoโ๏ธ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ๐ Files selected for processing (1)
๐ WalkthroughWalkthroughRoot layout for the Remain Application screen changed from a Column to a full-size Box; root uses Changes
Estimated code review effort๐ฏ 3 (Moderate) | โฑ๏ธ ~20 minutes Possibly related issues
Poem
๐ฅ 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 docstrings
๐งช 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.
๐งน Nitpick comments (2)
feature/src/main/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt (2)
70-71: Potential double bottom inset from combined safe-area modifiers.Using
systemBarsPadding()on the root andnavigationBarsPadding()on the bottom button can apply nav-bar inset twice, creating extra bottom gap on some devices. Consider using top-only inset on the root and keeping nav inset only on the CTA container.Suggested adjustment
-import androidx.compose.foundation.layout.systemBarsPadding +import androidx.compose.foundation.layout.statusBarsPadding ... Box( modifier = Modifier .fillMaxSize() .background(DmsTheme.colorScheme.background) - .systemBarsPadding(), + .statusBarsPadding(), ) {Also applies to: 127-129
๐ค Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/src/main/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt` around lines 70 - 71, The root Composable is using .systemBarsPadding() which combined with .navigationBarsPadding() on the CTA causes the navigation inset to be applied twice; change the root's inset to top-only (e.g., .statusBarsPadding() or .padding(WindowInsets.safeDrawing.only(WindowInsetsSides.Top))) and leave .navigationBarsPadding() only on the bottom CTA container (also update the other occurrence noted around the 127-129 area) so nav-bar inset is applied once; locate the root modifier call with .systemBarsPadding() and the CTA modifier with .navigationBarsPadding() in RemainApplicationScreen.kt and update accordingly.
98-100: Avoid hard-coded bottom reserve (120.dp) for CTA overlap prevention.
120.dpis brittle against CTA height/theme changes (e.g., text scale, design-token updates). Prefer a shared dimension token or measured CTA height to keep list clearance accurate over time.๐ค Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/src/main/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt` around lines 98 - 100, Replace the hard-coded bottom PaddingValues(bottom = 120.dp) used for contentPadding in RemainApplicationScreen with a shared dimension token or a dynamic measurement of the CTA's height (e.g., use a themed dimension resource or measure the composable/WindowInsets/Scaffold bottomBar height) so list clearance adapts to CTA/text-scale/design-token changes; update the usage around contentPadding / PaddingValues to reference the shared token or measured value instead of the literal 120.dp so future CTA height/theme changes automatically keep the list clear.
๐ค Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt`:
- Around line 70-71: The root Composable is using .systemBarsPadding() which
combined with .navigationBarsPadding() on the CTA causes the navigation inset to
be applied twice; change the root's inset to top-only (e.g.,
.statusBarsPadding() or
.padding(WindowInsets.safeDrawing.only(WindowInsetsSides.Top))) and leave
.navigationBarsPadding() only on the bottom CTA container (also update the other
occurrence noted around the 127-129 area) so nav-bar inset is applied once;
locate the root modifier call with .systemBarsPadding() and the CTA modifier
with .navigationBarsPadding() in RemainApplicationScreen.kt and update
accordingly.
- Around line 98-100: Replace the hard-coded bottom PaddingValues(bottom =
120.dp) used for contentPadding in RemainApplicationScreen with a shared
dimension token or a dynamic measurement of the CTA's height (e.g., use a themed
dimension resource or measure the composable/WindowInsets/Scaffold bottomBar
height) so list clearance adapts to CTA/text-scale/design-token changes; update
the usage around contentPadding / PaddingValues to reference the shared token or
measured value instead of the literal 120.dp so future CTA height/theme changes
automatically keep the list clear.
โน๏ธ Review info
โ๏ธ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 980b96f7-180e-4623-9a3f-c587117dbf76
๐ Files selected for processing (1)
feature/src/main/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt
There was a problem hiding this comment.
Actionable comments posted: 1
๐ค Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt`:
- Around line 98-100: The bottom padding in RemainApplicationScreen is
hard-coded as PaddingValues(bottom = 120.dp); instead derive it from navigation
insets plus a named reserved CTA height constant: read the nav bar inset (e.g.,
WindowInsets.navigationBars or LocalWindowInsets) and compute bottomPadding =
navInsetBottom + RESERVED_CTA_SPACE (declare RESERVED_CTA_SPACE as a clearly
named dp constant), then pass that into contentPadding instead of 120.dp so the
layout adapts to device insets and future CTA size changes.
๐ช Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
โน๏ธ Review info
โ๏ธ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19ac15d4-9388-4b6f-989c-d8d42e46697f
๐ Files selected for processing (1)
feature/src/main/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt
| contentPadding = PaddingValues( | ||
| bottom = 120.dp, | ||
| ), |
There was a problem hiding this comment.
Avoid hard-coded bottom list padding for an overlay CTA.
120.dp is tied to the current button/inset assumptions and can regress on devices with larger nav insets or future button size changes. Consider deriving bottom padding from nav insets plus a named reserved-space constant.
๐ก Suggested refactor
+import androidx.compose.foundation.layout.WindowInsets
+import androidx.compose.foundation.layout.asPaddingValues
+import androidx.compose.foundation.layout.calculateBottomPadding
+
+private val BottomButtonReservedSpace = 120.dp
+
LazyColumn(
modifier = Modifier
.fillMaxWidth()
.weight(1f)
.topPadding(30.dp)
.horizontalPadding(24.dp),
verticalArrangement = Arrangement.spacedBy(20.dp),
contentPadding = PaddingValues(
- bottom = 120.dp,
+ bottom = BottomButtonReservedSpace +
+ WindowInsets.navigationBars.asPaddingValues().calculateBottomPadding(),
),
) {๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contentPadding = PaddingValues( | |
| bottom = 120.dp, | |
| ), | |
| import androidx.compose.foundation.layout.WindowInsets | |
| import androidx.compose.foundation.layout.asPaddingValues | |
| import androidx.compose.foundation.layout.calculateBottomPadding | |
| private val BottomButtonReservedSpace = 120.dp | |
| LazyColumn( | |
| modifier = Modifier | |
| .fillMaxWidth() | |
| .weight(1f) | |
| .topPadding(30.dp) | |
| .horizontalPadding(24.dp), | |
| verticalArrangement = Arrangement.spacedBy(20.dp), | |
| contentPadding = PaddingValues( | |
| bottom = BottomButtonReservedSpace + | |
| WindowInsets.navigationBars.asPaddingValues().calculateBottomPadding(), | |
| ), | |
| ) { |
๐ค Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/remain/ui/RemainApplicationScreen.kt`
around lines 98 - 100, The bottom padding in RemainApplicationScreen is
hard-coded as PaddingValues(bottom = 120.dp); instead derive it from navigation
insets plus a named reserved CTA height constant: read the nav bar inset (e.g.,
WindowInsets.navigationBars or LocalWindowInsets) and compute bottomPadding =
navInsetBottom + RESERVED_CTA_SPACE (declare RESERVED_CTA_SPACE as a clearly
named dp constant), then pass that into contentPadding instead of 120.dp so the
layout adapts to device insets and future CTA size changes.
1 participant
๊ฐ์
์์ ์ฌํญ
์ถ๊ฐ ๋ก ํ ๋ง
Summary by CodeRabbit
Bug Fixes
Refactor