fix(just-lift): cap wheel drag hot zone + side-by-side weight cards on iPhone portrait (#571) - #574
Conversation
…n iPhone portrait (#571) Issue #571 reporter (iPhone 16 Pro, Just Lift, LB) saw the Weight Change Per Rep slider produce 29/30/44-49 instead of 1 lb increments. RCA proved the slider is correctly bound to valueRange = -10..+10 with 1 lb steps and cannot emit those values; the visible jumps were the iOS CompactNumberPicker (LazyColumn wheel, range 1..242 LB) claiming vertical drags intended for the slider and snapping to neighbour weights. This commit applies the three-part fix from the GPT-5.5 RCA: 1. WindowSizeClass.kt + JustLiftScreen.kt — split the side-by-side weight card gate from the broader useCompactAccessibility. New shouldStackWeightCards() returns true only when the screen is also short (height Compact), so iPhone portrait (402x874dp) with default Dynamic Type / Bold Text keeps the two weight cards side-by-side. iPhone landscape and short-height tablets still stack. 2. JustLiftScreen.kt — add an explicit Spacer(Spacing.small) between the two weight cards when stacked, belt-and-braces separation. 3. CompactNumberPicker.ios.kt — add a pointerInput on the wheel's BoxWithConstraints that consumes vertical drags landing outside the centred 36dp selected band, so the inner LazyColumn does not claim them. In edit mode the wheel defers entirely to the BasicTextField. Adds WindowSizeClassTest cases for shouldStackWeightCards and a new JustLiftScreenWeightSliderWiringTest that pins the ProgressionSlider's valueRange and weightChangePerRep binding against future regressions. Refs: #571 (Fixes) RCA: rca_owner=gpt-5.5-xhigh, fix_driver=gpt55_rca
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a gesture conflict issue (#571) between the iOS wheel picker and the progression slider on iPhone portrait screens by keeping the weight cards side-by-side unless the device height is compact. It also restricts the wheel's vertical drag zone and adds regression tests. The reviewer feedback highlights several critical areas for improvement: a usability issue where consuming pointer events outside the center band creates a scroll 'dead zone' (suggesting a nested scrolling approach instead); a layout spacing miscalculation where an explicit spacer combined with spacedBy results in excessive gaps; redundant logic in shouldStackWeightCards that can be simplified; and a fragile testing anti-pattern of reading source files from disk in unit tests, which should be replaced with Compose UI tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| .pointerInput(isEditing, itemHeight) { | ||
| if (isEditing) return@pointerInput | ||
| val itemHeightPx: Float = itemHeight.toPx() | ||
| val centerY: Float = size.height.toFloat() / 2f | ||
| awaitEachGesture { | ||
| val down = awaitFirstDown(requireUnconsumed = false) | ||
| val offset: Float = down.position.y - centerY | ||
| val absOffset: Float = if (offset < 0f) -offset else offset | ||
| val inCenterBand: Boolean = absOffset <= itemHeightPx / 2f | ||
| if (inCenterBand) { | ||
| // Let the inner LazyColumn handle this gesture as before. | ||
| return@awaitEachGesture | ||
| } | ||
| // Outside the centred band: consume the down so the LazyColumn | ||
| // does not claim it. The outer verticalScroll / sibling card | ||
| // can then take the drag. | ||
| down.consume() | ||
| // Track until release so we don't get re-entered mid-gesture. | ||
| while (true) { | ||
| val event = awaitPointerEvent() | ||
| if (event.changes.all { !it.pressed }) break | ||
| // Consume movement too so parent verticalScroll's nested | ||
| // scroll connection does not interfere. | ||
| event.changes.forEach { it.consume() } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Fixed in 0ee7b27. Replaced the consume-events approach with a dragStartedInCenterBand state flag that gates LazyColumn.userScrollEnabled. When the drag starts outside the centred row, the LazyColumn no longer claims the gesture, so the outer verticalScroll / sibling ProgressionSlider can take over naturally — no event consumption, no dead zone. The pointerInput now just observes the down position and toggles the flag; it waits for the gesture to finish then resets the flag for the next drag.
| if (stackWeightCards) { | ||
| Spacer(modifier = Modifier.height(Spacing.small)) | ||
| } |
There was a problem hiding this comment.
Fixed in 0ee7b27. Moved the additional separation to Modifier.padding(bottom = Spacing.small) on the first card (only when stackWeightCards is true), so the outer Column's Arrangement.spacedBy(Spacing.medium) does not double the gap. Total gap is now 16dp (outer) + 8dp (this padding) = 24dp, matching the original PR description.
| fun shouldStackWeightCards( | ||
| windowSizeClass: WindowSizeClass, | ||
| fontScale: Float, | ||
| boldTextEnabled: Boolean, | ||
| fontScaleThreshold: Float = 1.15f, | ||
| ): Boolean = shouldUseCompactAccessibilityLayout( | ||
| windowSizeClass = windowSizeClass, | ||
| fontScale = fontScale, | ||
| boldTextEnabled = boldTextEnabled, | ||
| fontScaleThreshold = fontScaleThreshold, | ||
| ) && windowSizeClass.heightSizeClass == WindowHeightSizeClass.Compact | ||
|
|
||
| /** | ||
| * Issue #571: Composable convenience wrapper around [shouldStackWeightCards] that reads | ||
| * the current WindowSizeClass, font scale, and accessibility settings from the ambient | ||
| * composition locals. Use this in Composables; use the pure function in unit tests. | ||
| */ | ||
| @Composable | ||
| fun useStackedWeightCardsLayout(): Boolean = shouldStackWeightCards( | ||
| windowSizeClass = LocalWindowSizeClass.current, | ||
| fontScale = LocalDensity.current.fontScale, | ||
| boldTextEnabled = LocalPlatformAccessibilitySettings.current.boldTextEnabled, | ||
| ) |
There was a problem hiding this comment.
Fixed in 0ee7b27. You were right — shouldUseCompactAccessibilityLayout already returns true for compact heights via its first OR-clause, so the && height == Compact made the LHS redundant. Reduced the helper to heightSizeClass == Compact and dropped the dead fontScale / boldTextEnabled / fontScaleThreshold parameters from the public signature. WindowSizeClassTest cases were updated to match the simplified contract, and a new exhaustive test (stackWeightCards_heightIsTheSoleDeterminant) pins the contract that height is the only input.
| private fun readJustLiftScreenSource(): String { | ||
| // Read from the classpath via the resources directory, or fall back to the | ||
| // project root. The test is allowed to read the source file because it | ||
| // runs in the commonTest source set of the same Gradle module that owns | ||
| // JustLiftScreen.kt. Working directory resolution: try the explicit | ||
| // path first, then walk up to find a gradle root. | ||
| val relativePath = | ||
| "src/commonMain/kotlin/com/devil/phoenixproject/presentation/screen/JustLiftScreen.kt" | ||
| val candidates = mutableListOf<java.io.File>() | ||
| candidates.add(java.io.File(relativePath)) | ||
| // Walk up looking for a `shared` or `.git` marker to anchor the search. | ||
| var dir: java.io.File? = java.io.File(".").absoluteFile | ||
| repeat(6) { | ||
| if (dir == null) return@repeat | ||
| candidates.add(java.io.File(dir, "shared/$relativePath")) | ||
| if (java.io.File(dir, ".git").exists() || java.io.File(dir, "settings.gradle.kts").exists()) { | ||
| candidates.add(java.io.File(dir, relativePath)) | ||
| } | ||
| dir = dir.parentFile | ||
| } | ||
| for (file in candidates) { | ||
| if (file.exists()) return file.readText() | ||
| } | ||
| error( | ||
| "Could not locate JustLiftScreen.kt on disk. Searched: " + | ||
| candidates.joinToString(", ") { it.path } + | ||
| ". Run the test from the shared/ module's working directory.", | ||
| ) | ||
| } |
There was a problem hiding this comment.
You're right that this is a fragile-test anti-pattern. Kept the static-analysis approach for this PR and added a "Why static analysis instead of Compose UI tests?" section to the test file's KDoc that documents: (1) the bug class is "widget wired wrong" — a UI test would still pass if onValueChange were silently rebinded to weightPerCable; (2) the repo doesn't currently have a Compose UI test harness for JustLiftScreen and adding one is out of scope for the #571 fix; (3) the assertions are scoped to a small set of exact strings so failure messages stay clear. The KDoc ends with a migration note pointing at when a Compose UI test should replace these static assertions. Happy to take that as a follow-up issue if you want it tracked separately.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| * (width Compact) unless the height is also Compact — the only compact-width+short-height | ||
| * case (iPhone landscape, 740×390dp) genuinely needs the stacked layout to fit. | ||
| * | ||
| * Tablets (width Medium/Expanded) always keep the side-by-side layout. |
There was a problem hiding this comment.
🔥 The Roast: The KDoc says "Tablets (width Medium/Expanded) always keep the side-by-side layout," but the implementation and tests stack short-height tablets with the confidence of someone who has not read their own documentation. This paragraph is cosplaying as truth.
🩹 The Fix: Update the KDoc to match the actual rule: stack whenever heightSizeClass == WindowHeightSizeClass.Compact (including short-height tablets), or change the implementation/tests if tablets really must stay side-by-side.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // Let the inner LazyColumn handle this gesture as before. | ||
| return@awaitEachGesture | ||
| } | ||
| // Outside the centred band: consume the down so the LazyColumn |
There was a problem hiding this comment.
🔥 The Roast: Consuming the down event outside the center band stops the inner LazyColumn from claiming the gesture, but it may also block an ancestor verticalScroll from taking over if that parent has not already claimed it. "Outer scroll can take over" is doing a heroic impression of certainty for a line that just yelled "mine."
🩹 The Fix: Verify the stacked-layout gesture pipeline on device/simulator. If outside-band drags should scroll the screen, avoid consuming in a way that prevents the parent scroll from handling them, or document the exact parent/child gesture contract this depends on.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| package com.devil.phoenixproject.presentation | ||
|
|
||
| import kotlin.test.Test | ||
| import kotlin.test.assertEquals |
There was a problem hiding this comment.
🔥 The Roast: assertEquals is imported but never used. It is standing in the import section like a party guest who left before anyone arrived, and ktlint may still notice the empty chair.
🩹 The Fix:
| import kotlin.test.assertEquals | |
| import kotlin.test.assertTrue |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR actually cleaned up cleanly again. The incremental diff is a surgical extraction of inline file-path spelunking into a proper 📊 Overall: Like watching someone finally pull a 30-line helper out of a test method — same behavior, but now I don't have to trace directory walks just to understand the assertion. Annoyingly competent. Files Reviewed (4 new files)
Files Reviewed from Previous (carried forward)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit 0ee7b27)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0ee7b27)Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR actually cleaned up cleanly. I had my skepticism hat on, but the changes address the previous concerns with surgical precision. 📊 Overall: Like watching a chef fix a dish that was almost right — the original had good bones, and these tweaks make it actually good. The height-only stacking logic for weight cards is cleaner than the previous fontScale/boldText tangle, and the gesture passthrough is implemented without the event consumption landmines I initially feared. Files Reviewed (5 files)
Previous review (commit db589c2)Verdict: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The pure 💀 Worst part: The pointer gesture handoff may be the least certain part of the fix: it stops the wheel from claiming drags, but the code may also prevent the ancestor scroll from taking over. 📊 Overall: Like a car with a great new steering wheel but one suspicious brake line: the direction is right, but I would not merge it without checking the gesture plumbing. Fix these issues in Kilo Cloud Other Observations
Files Reviewed (5 files)
Reviewed by step-3.7-flash-20260528 · 197,565 tokens |
Three valid findings from @gemini-code-assist[bot] on PR #574 (#571), plus a documentation comment for the fourth: 1. **Dead-zone fix (HIGH):** the previous commit's pointerInput consumed every pointer event outside the wheel's centred 36dp band, which prevented the outer verticalScroll from scrolling while the user's finger was on those top/bottom areas of the wheel. Replaced the consume-events approach with a 'dragStartedInCenterBand' state flag that gates LazyColumn.userScrollEnabled. The inner LazyColumn no longer claims the gesture when the drag starts outside the centred row, so the outer verticalScroll / sibling ProgressionSlider can take over naturally. No event consumption, no dead zone. 2. **Spacing fix (MEDIUM):** the explicit Spacer(modifier = Modifier.height( Spacing.small)) added between the two stacked cards was being double-spaced by the outer Column's Arrangement.spacedBy(Spacing.medium) — total gap was 40dp, not the 24dp the PR description claimed. Moved the additional separation to bottom padding on the first card (only when stackWeightCards is true), so the outer arrangement no longer doubles up: 16dp (outer) + 8dp (this padding) = 24dp. 3. **Redundant logic (MEDIUM):** shouldStackWeightCards was defined as 'shouldUseCompactAccessibilityLayout(...) && heightSizeClass == Compact', but shouldUseCompactAccessibilityLayout already returns true for compact heights (its first OR-clause), so the entire LHS was redundant. Reduced the helper to 'heightSizeClass == Compact'. fontScale, boldTextEnabled, and fontScaleThreshold parameters are now gone from the public signature. Updated WindowSizeClassTest cases to match the simplified contract; added a new exhaustive test pinning height as the sole determinant. 4. **Fragile test (deferred with rationale):** the static-analysis approach in JustLiftScreenWeightSliderWiringTest is a known anti-pattern. Documented in the file's KDoc why we keep static analysis for this PR (the bug class is 'widget wired wrong' — a UI test would still pass if onValueChange were silently rebinded) and when it should be migrated to a Compose UI test. Local gates (all green): - :shared:testAndroidHostTest → BUILD SUCCESSFUL - :shared:lintAnalyzeAndroidHostTest → BUILD SUCCESSFUL - :shared:compileKotlinIosArm64 → BUILD SUCCESSFUL
The 'iOS Test Target Compile' CI job failed because the static-analysis test introduced in #574 uses java.io.File, which is JVM-only and not available under the iOS test target. The Android test target compiles fine because it links the JVM stdlib. Move the file-reading logic into a KMP expect/actual pair: - commonTest/testutil/SourceFileReader.kt → expect fun readProjectFile - androidHostTest/testutil/SourceFileReader.android.kt → actual via java.io.File - iosTest/testutil/SourceFileReader.ios.kt → actual via NSFileManager + NSString.stringWithContentsOfFile The test file itself now just calls readProjectFile(relativePath) and asserts non-null. All the up-walk logic is contained in the actuals, so the iOS implementation can be re-tuned independently if Xcode's test-runner cwd differs from the gradle one. Also drops the unused 'assertEquals' import that was added in the previous commit (assertEquals is still used in WindowSizeClassTest.kt; that file keeps its import). Local gates (all green): - :shared:compileTestKotlinIosArm64 → BUILD SUCCESSFUL - :shared:compileKotlinIosArm64 → BUILD SUCCESSFUL - :shared:testAndroidHostTest → BUILD SUCCESSFUL - :shared:lintAnalyzeAndroidHostTest → BUILD SUCCESSFUL
Fixes #571
Problem
Reporter (iPhone 16 Pro, Just Lift, LB) saw the Weight Change Per Rep slider produce 29/30/44-49 instead of 1 lb increments. The RCA in #571 (comment by @9thLevelSoftware) proved the slider is correctly bound to
valueRange = -10f..10fwithsteps = 19(i.e. 21 stops → step size 1 lb) and cannot emit those values. The visible jumps came from the iOSCompactNumberPicker(LazyColumnwheel, range 1..242 LB) claiming vertical drags intended for the slider and snapping to neighbour weights.Root cause
useCompactAccessibility = trueon iPhone 16 Pro portrait (402×874dp) with default Dynamic Type and Bold Text off —JustLiftScreen.kt:289.JustLiftScreen.kt:404, 460then dropModifier.weight(1f)and stack the two weight cards in a single vertical column.Spacing.medium(16dp) between them, and the wheel'sBoxWithConstraintsisitemHeight * 3 = 108dptall.LazyColumnclaims vertical drags anywhere inside its 108dp container, so drags starting on (or near) the slider thumb are routed to the wheel and snap to neighbour weights like 44/45/46.Fix (3 parts, per the RCA contract)
1. Decouple the side-by-side weight card gate from
useCompactAccessibilityNew
shouldStackWeightCards()inWindowSizeClass.ktreturns true only when the screen is also short (heightSizeClass == Compact). New composable wrapperuseStackedWeightCardsLayout()reads the currentLocalWindowSizeClass,LocalDensity, andLocalPlatformAccessibilitySettings.JustLiftScreen.ktnow gates the Weight per Cable and Weight Change Per Rep cards onstackWeightCards, notuseCompactAccessibility.Result: on iPhone 16 Pro portrait (default Dynamic Type, Bold Text off), the two cards are side-by-side and the wheel cannot land above the slider by construction.
2. Cap the wheel's vertical drag hot zone to the centred 36dp band
CompactNumberPicker.ios.kt: add aModifier.pointerInput(isEditing, itemHeight) { awaitEachGesture { ... } }on the wheel'sBoxWithConstraints. WhenisEditing = false, drags landing within±itemHeight/2of the container center pass through to the innerLazyColumnas before. Drags landing outside the centred band are consumed at the wheel level so theLazyColumndoes not claim them — the outerverticalScrolland the sibling slider card can then take the drag. In edit mode the wheel defers entirely to theBasicTextField.3. Add an explicit vertical
Spacerbetween the two weight cards when stackedJustLiftScreen.kt: whenstackWeightCards, insertSpacer(Modifier.height(Spacing.small))between the Weight per Cable and Weight Change Per Rep cards. Combined with the outer column'sSpacing.medium, total separation is 24dp (Spacing.large) — belt-and-braces for any future regression in the wheel's hot-zone cap.Acceptance criteria (RCA)
JustLiftScreenWeightSliderWiringTest.Non-goals (RCA non-goals)
ProgressionSlider'svalueRange(it was already -10..+10).UIPickerView(PR [Bug]: Just Lift weight selection bug #359 documented why).isEditingtoggle on the slider (not the bug).useCompactAccessibilitygate — the outerverticalScrolland the other cards still use it.Tests
WindowSizeClassTestcases pinshouldStackWeightCardsbehavior (iPhone 16 Pro portrait default + Bold Text on both keep side-by-side; iPhone landscape 740×390dp + short-height tablet both stack; tall tablet never stacks;stack ⊆ compactinvariant).JustLiftScreenWeightSliderWiringTestpins:valueRange = -10f..10fon theProgressionSlidercall site,onValueChangewrites toweightChangePerRepand notweightPerCable,useStackedWeightCardsLayout()/shouldStackWeightCards,verticalScroll(contentScrollState)still uses the broader gate.:shared:testAndroidHostTestpasses (16/16 targeted + unchanged existing tests).:shared:compileKotlinIosArm64passes.:shared:lintAnalyzeAndroidHostTestpasses.RCA
rca_owner: gpt-5.5-xhighrca_model: gpt-5.5rca_provider: openai-codexfix_driver: gpt55_rca~/.hermes/phoenix-bug-recreations/571/bug-recreation.md