ui: extract shared sign-policy option row and fix doubled card border (#403)#418
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughSign-policy cards are consolidated into a shared ChangesSign-policy UI consolidation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant OnboardingScreen
participant SignPolicyScreen
participant SignPolicyOptionRow
participant KeepCard
User->>OnboardingScreen: select policy
OnboardingScreen->>SignPolicyOptionRow: provide selected policy state
User->>SignPolicyScreen: select policy
SignPolicyScreen->>SignPolicyOptionRow: provide selected policy state
SignPolicyOptionRow->>KeepCard: render selectable bordered card
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/io/privkey/keep/nip55/SignPolicyScreen.kt (1)
63-71: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrevent race conditions when persisting the global policy.
Launching multiple uncoordinated coroutines on
Dispatchers.IOwhen the user rapidly taps different options can lead to a race condition. The asynchronous writes might execute out of order, resulting in the persisted global policy being out of sync with the UI'sselectedPolicy.To guarantee data integrity, cancel the previous save job before starting a new one.
🔒 Proposed fix using job cancellation
First, declare a reference to track the active job at the top of the composable:
+import kotlinx.coroutines.Job ... var selectedPolicy by remember { mutableStateOf(SignPolicy.MANUAL) } var userInteracted by remember { mutableStateOf(false) } val coroutineScope = rememberCoroutineScope() + var saveJob by remember { mutableStateOf<Job?>(null) }Then, update the
onSelectedcallback to cancel any pending write:onSelected = { userInteracted = true selectedPolicy = policy + saveJob?.cancel() - coroutineScope.launch { + saveJob = coroutineScope.launch { withContext(Dispatchers.IO) { signPolicyStore.setGlobalPolicy(policy) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/io/privkey/keep/nip55/SignPolicyScreen.kt` around lines 63 - 71, Track the active persistence coroutine in SignPolicyScreen and cancel it before launching a new save in the onSelected callback. Update the job reference to the newly launched coroutine while preserving the existing IO-backed signPolicyStore.setGlobalPolicy(policy) operation, ensuring rapid selections persist in order.
🧹 Nitpick comments (2)
app/src/main/kotlin/io/privkey/keep/ui/components/SignPolicyOptionRow.kt (1)
29-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClip the ripple effect to match the card's rounded corners.
Applying
Modifier.selectableoutside theCardmeans its ripple effect (indication) is drawn on the rectangular layout bounds, ignoring the card's rounded corners. To fix this visual glitch, clip the modifier to the card's shape before applying the selectable behavior.♻️ Proposed fix
+import androidx.compose.material3.CardDefaults +import androidx.compose.ui.draw.clip ... KeepCard( - modifier = modifier.selectable( + modifier = modifier + .clip(CardDefaults.shape) + .selectable( selected = isSelected, onClick = onSelected, role = Role.RadioButton ), border = BorderStroke(Dimens.cardBorderWidth, borderColor)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/io/privkey/keep/ui/components/SignPolicyOptionRow.kt` around lines 29 - 35, Update the modifier chain on KeepCard in SignPolicyOptionRow so the card shape clips its bounds before selectable applies the ripple indication. Reuse the card’s existing rounded shape for the clip, preserving the current selection, click, and border behavior.app/src/main/kotlin/io/privkey/keep/OnboardingScreen.kt (1)
105-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
selectableGroup()for proper accessibility grouping.Both screens display a list of radio-button options, but the parent layouts lack
Modifier.selectableGroup(). Without this modifier, screen readers will not correctly announce the options as part of a single interactive radio group (e.g., "1 of 3").Wrap the
SignPolicy.entries.forEachloops in aColumnwithModifier.selectableGroup()to ensure proper accessibility semantics.
app/src/main/kotlin/io/privkey/keep/OnboardingScreen.kt#L105-L114: Wrap the list in aColumn(Modifier.selectableGroup(), verticalArrangement = Arrangement.spacedBy(Dimens.space16))app/src/main/kotlin/io/privkey/keep/nip55/SignPolicyScreen.kt#L59-L73: Wrap the list in aColumn(Modifier.selectableGroup(), verticalArrangement = Arrangement.spacedBy(12.dp))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/io/privkey/keep/OnboardingScreen.kt` around lines 105 - 114, Wrap the SignPolicy.entries option list in OnboardingScreen.kt (lines 105-114) with a Column using Modifier.selectableGroup() and Arrangement.spacedBy(Dimens.space16). Apply the same change to SignPolicyScreen.kt (lines 59-73), using Arrangement.spacedBy(12.dp), while preserving each SignPolicyOptionRow and its selection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/src/main/kotlin/io/privkey/keep/nip55/SignPolicyScreen.kt`:
- Around line 63-71: Track the active persistence coroutine in SignPolicyScreen
and cancel it before launching a new save in the onSelected callback. Update the
job reference to the newly launched coroutine while preserving the existing
IO-backed signPolicyStore.setGlobalPolicy(policy) operation, ensuring rapid
selections persist in order.
---
Nitpick comments:
In `@app/src/main/kotlin/io/privkey/keep/OnboardingScreen.kt`:
- Around line 105-114: Wrap the SignPolicy.entries option list in
OnboardingScreen.kt (lines 105-114) with a Column using
Modifier.selectableGroup() and Arrangement.spacedBy(Dimens.space16). Apply the
same change to SignPolicyScreen.kt (lines 59-73), using
Arrangement.spacedBy(12.dp), while preserving each SignPolicyOptionRow and its
selection behavior.
In `@app/src/main/kotlin/io/privkey/keep/ui/components/SignPolicyOptionRow.kt`:
- Around line 29-35: Update the modifier chain on KeepCard in
SignPolicyOptionRow so the card shape clips its bounds before selectable applies
the ripple indication. Reuse the card’s existing rounded shape for the clip,
preserving the current selection, click, and border behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b9d844e-6d54-41f5-8f25-d000e6fb2cb7
📒 Files selected for processing (4)
app/src/main/kotlin/io/privkey/keep/OnboardingScreen.ktapp/src/main/kotlin/io/privkey/keep/nip55/SignPolicyScreen.ktapp/src/main/kotlin/io/privkey/keep/ui/components/KeepCard.ktapp/src/main/kotlin/io/privkey/keep/ui/components/SignPolicyOptionRow.kt
kwsantiago
force-pushed
the
sign-policy-option-row
branch
from
1a97874 to
84f2b8d
Compare
2 participants
Summary by CodeRabbit
New Features
UI Improvements