feat: rebuild sample app as an 8-screen haptic verification harness - #86
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 16 minutes and 39 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (45)
📝 WalkthroughWalkthroughAdds amplitude control to ChangesHapticExecutor amplitude control
Haptic capability snapshot API
Sample app rebuild
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Jindong Core Test Coverage Report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
jindong-compose/src/commonTest/kotlin/io/github/compose/jindong/HapticCapabilitiesTest.kt (1)
22-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a composition-level test for
rememberHapticCapabilities().This only verifies that
HapticCapabilitiesstores constructor args. It does not exercise the newrememberHapticCapabilities()path, so a regression in readingLocalHapticExecutoror resolving the platform would still pass. Add a provider-backed test with a fake executor and assert the returned snapshot.🤖 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 `@jindong-compose/src/commonTest/kotlin/io/github/compose/jindong/HapticCapabilitiesTest.kt` around lines 22 - 37, Add a composition-level test that exercises rememberHapticCapabilities() instead of only testing the HapticCapabilities constructor. Use a provider-backed setup with a fake LocalHapticExecutor to verify the composable reads from composition and resolves the platform correctly, then assert the returned snapshot values. Keep the existing HapticCapabilitiesTest focused on constructor behavior, and add a new test case that names rememberHapticCapabilities() and LocalHapticExecutor so the regression path is covered.
🤖 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.
Inline comments:
In
`@jindong-core/src/commonMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.kt`:
- Around line 37-47: The new hasAmplitudeControl member on HapticExecutor is a
breaking API change for downstream implementers; if this release should preserve
compatibility, move the capability behind a separate interface or provide a
defaulted way to expose it. Update HapticExecutor and any related
implementations/helpers that reference the capability so existing
implementations do not need to add new members immediately.
In
`@jindong-core/src/iosMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.ios.kt`:
- Around line 58-60: The iOS haptic capabilities currently report amplitude
control as always true in HapticExecutor, which creates an invalid state when
Core Haptics is unavailable. Update the HapticExecutor.ios implementation so
hasAmplitudeControl is only true when the device is actually supported, and make
sure rememberHapticCapabilities and any UI badges relying on it use the real
support state instead of a hard-coded constant.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/Chip.kt`:
- Around line 59-70: Move the click handling from the inner Column/Row onto the
outer Box in the Chip composables so the full `Dimens.minTouch` area is
tappable. Update the chip implementations identified by `Chip` and the shared
clickable modifier chain to keep the visual styling on the inner content but
apply `clickable(onClick = onClick)` to the min-touch container. Also apply the
same fix to the other chip variant referenced by the comment so both
implementations use the larger hit target.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/JindongScaffold.kt`:
- Around line 145-155: Update the clickable controls in JindongScaffold to
expose proper button semantics: add role = Role.Button to both the back action
and the other clickable control, and in the back icon block set the Icon
contentDescription to null so the label is not announced twice. Locate the back
control and icon setup in JindongScaffold and adjust the existing
clickable/semantics chain rather than adding new UI.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/ScreenPieces.kt`:
- Around line 84-96: The zero-decimal branch in formatFixed currently truncates
by calling value.toLong(), which is inconsistent with the rounding used in the
rest of the helper. Update the decimals <= 0 path in formatFixed to apply the
same half-up rounding before converting to a whole-number string, so values like
1.9f format consistently with the fractional case.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/SegmentedToggle.kt`:
- Around line 52-70: Each segment in SegmentedToggle should have its own 48dp
minimum tap target instead of relying on the parent Row. Move the minimum
height/touch enforcement onto the child segment Box in the forEachIndexed loop,
alongside the clickable modifier, so short labels still produce a full-size hit
area. Keep the existing row spacing and selection styling, but ensure each
segment wrapper independently respects Dimens.minTouch.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/nav/AppRoot.kt`:
- Around line 41-45: Restore Android system-back handling in AppRoot so the back
gesture/button navigates from the current screen back to Screen.Home instead of
exiting the app. Keep the existing app-bar chevron behavior in JindongScaffold
as a fallback, but add an Android-specific back hook in the AppRoot host when
current != Screen.Home, using the existing navigation state and screen switch
logic.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/IntensityLabScreen.kt`:
- Around line 271-285: The PlayChip touch target is split across two nodes, so
the advertised 48dp area is not actually clickable. Update PlayChip so the
minimum size and the clickable handler live on the same modifier chain/node (the
one that draws the bordered pill), and keep the outer wrapper only if it is no
longer needed for hit testing.
- Around line 148-155: The Play all sequence in Jindong(playAll) does not match
the staircase timeline because it uses a longer inter-pulse gap than the
rendered pattern. Update the Sequence playback logic so the Delay between Haptic
events matches the same timing used by the timeline/staircase rendering, keeping
ascendingPresets in sync with the visual preview.
- Around line 159-160: The custom haptic preview in IntensityLabScreen should
not auto-play when the custom slider changes. Update the Jindong(...) usage tied
to ilCustom so slider movement only updates state and does not retrigger
Haptic(100.ms, HapticIntensity.Custom(ilCustom)); keep playback gated behind the
explicit Play control in CustomCard, similar to the other non-reactive modules.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/PresetGalleryScreen.kt`:
- Around line 195-205: The play chip’s touch target is still too small because
only the inner clickable Box in PresetGalleryScreen is interactive while the
outer defaultMinSize wrapper is not. Move the clickable modifier to the outer
Box (or otherwise make the 48dp container itself handle clicks) so the full
minimum touch area is tappable, while preserving the inner visual styling with
clip/border/padding.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/ReactiveScreen.kt`:
- Around line 117-128: The regression oracle is ineffective because `CaseCard`
receives `actual` from the same `expectedActual` state used for `expected`, so
failures in `singleBar`, `reactiveCountPattern`, or `TimelineMapper.toBars`
cannot be detected. Update the affected `CaseCard` usages in `ReactiveScreen` so
`actual` is derived from the rendered output/state independently of the expected
value, or change these fields to explicit explanatory labels if they are not
meant to be assertions.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/RepeatScreen.kt`:
- Around line 168-177: The repeatPattern() helper is offsetting each
ScheduledHapticEvent by a fixed +10ms, which makes the preview timeline diverge
from the actual Repeat { Haptic(); Delay() } playback starting at 0ms. Update
repeatPattern() in RepeatScreen so the generated start times align with the real
replayed pattern (keep the same 150ms spacing, but remove the artificial offset)
and ensure HapticPattern is built from the corrected event timestamps.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/RepeatWithIndexScreen.kt`:
- Around line 212-224: The preview/playback timing is inconsistent in
repeatIndexPattern, since the helper currently offsets each ScheduledHapticEvent
by 10ms while the RepeatWithIndex DSL path starts at 0ms. Update
repeatIndexPattern to use the same start-time scheme as the DSL-generated
timeline so both paths schedule events identically, and keep the change
localized to the repeatIndexPattern helper and any matching RepeatWithIndex
screen preview logic.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/SingleHapticScreen.kt`:
- Around line 182-188: The Jindong block in SingleHapticScreen is keyed on
sgDur, sgChip, and sgInt as well as playTrigger, which makes haptics re-run on
slider/chip edits instead of only on Play. Update the Jindong invocation so
execution is driven only by playTrigger, while still reading the live state
values inside the block for Haptic and resolvedIntensity. Keep the screen state
updates intact, but remove the non-Play keys from the Jindong call.
In
`@sample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/TimingScreen.kt`:
- Around line 191-200: The click target is attached to the inner chip Box
instead of the outer min-sized container, so the tappable area does not match
the intended touch target. Update the TimingScreen layout so the outer Box with
defaultMinSize(minHeight = Dimens.minTouch) also owns the clickable/onClick
behavior, and keep the inner Box only for clipping/border/visual styling. This
ensures the add-node affordance uses the full minimum touch area while
preserving the chip appearance.
---
Nitpick comments:
In
`@jindong-compose/src/commonTest/kotlin/io/github/compose/jindong/HapticCapabilitiesTest.kt`:
- Around line 22-37: Add a composition-level test that exercises
rememberHapticCapabilities() instead of only testing the HapticCapabilities
constructor. Use a provider-backed setup with a fake LocalHapticExecutor to
verify the composable reads from composition and resolves the platform
correctly, then assert the returned snapshot values. Keep the existing
HapticCapabilitiesTest focused on constructor behavior, and add a new test case
that names rememberHapticCapabilities() and LocalHapticExecutor so the
regression path is covered.
🪄 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: 07e1c527-25a1-4fd2-b1b0-1448e4d9a375
⛔ Files ignored due to path filters (4)
sample/shared/src/commonMain/composeResources/font/ibm_plex_mono_medium.ttfis excluded by!**/*.ttfsample/shared/src/commonMain/composeResources/font/ibm_plex_mono_regular.ttfis excluded by!**/*.ttfsample/shared/src/commonMain/composeResources/font/ibm_plex_mono_semibold.ttfis excluded by!**/*.ttfsample/shared/src/commonMain/composeResources/font/instrument_sans_variable.ttfis excluded by!**/*.ttf
📒 Files selected for processing (41)
jindong-compose/api/jindong-compose.apijindong-compose/api/jindong-compose.klib.apijindong-compose/src/androidMain/kotlin/io/github/compose/jindong/HapticCapabilities.android.ktjindong-compose/src/commonMain/kotlin/io/github/compose/jindong/HapticCapabilities.ktjindong-compose/src/commonTest/kotlin/io/github/compose/jindong/HapticCapabilitiesTest.ktjindong-compose/src/iosMain/kotlin/io/github/compose/jindong/HapticCapabilities.ios.ktjindong-core/api/jindong-core.apijindong-core/api/jindong-core.klib.apijindong-core/src/androidMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.android.ktjindong-core/src/commonMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.ktjindong-core/src/commonTest/kotlin/io/github/compose/jindong/core/executor/HapticExecutorCapabilityTest.ktjindong-core/src/commonTest/kotlin/io/github/compose/jindong/core/fake/FakeHapticExecutor.ktjindong-core/src/iosMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.ios.ktsample/shared/src/commonMain/composeResources/files/IBMPlexMono-OFL.txtsample/shared/src/commonMain/composeResources/files/InstrumentSans-OFL.txtsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/SampleApp.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/CapabilityBadge.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/Chip.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/HapticTimeline.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/JindongIcons.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/JindongScaffold.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/LabeledSlider.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/MonoLabel.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/PlayButton.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/ScreenPieces.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/components/SegmentedToggle.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/nav/AppRoot.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/nav/Screen.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/HarnessLogic.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/HomeScreen.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/IntensityLabScreen.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/PresetGalleryScreen.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/ReactiveScreen.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/RepeatScreen.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/RepeatWithIndexScreen.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/SingleHapticScreen.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/screens/TimingScreen.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/theme/Dimens.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/theme/JindongColors.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/theme/JindongTheme.ktsample/shared/src/commonMain/kotlin/io/github/compose/jindong/sample/theme/JindongTypography.kt
l2hyunwoo
force-pushed
the
feature/sample-redesign
branch
from
June 28, 2026 11:54
9fc7146 to
6e858de
Compare
wisemuji
approved these changes
Jul 3, 2026
wisemuji
left a comment
wisemuji
left a comment
Contributor
There was a problem hiding this comment.
This looks amazing!!! 🚀
This was referenced Jul 8, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rebuilds the sample app from a single preset list into an 8-screen verification harness, where each screen isolates one DSL feature and renders it on a shared timeline so the haptic is visible, not just felt. Adds a small public capability read API the home screen needs, and bundles the handoff's fonts.
Design source: a high-fidelity handoff (tokens, per-screen specs, an interactive prototype). The work ports that design to Compose Multiplatform on the existing Material3/own-theme patterns.
What changed
HapticCapabilitiesread API (jindong-compose): publicHapticCapabilities(isSupported, hasAmplitudeControl, platform)+HapticPlatformenum +rememberHapticCapabilities().hasAmplitudeControlis promoted onto theHapticExecutorinterface (Android readsVibrator.hasAmplitudeControl(); iOS Core Haptics is continuous, sotrue).LocalHapticExecutorstays internal. API dumps regenerated.sample/shared): light/dark color tokens, typography (Instrument Sans for UI, IBM Plex Mono for numbers and section labels), spacing/radius, all exposed through aJindongThemewithstaticCompositionLocalOf.JindongScaffold(56dp app bar + scrolling body),HapticTimeline(a single Canvas drawing one bar per event plus a playhead),CapabilityBadge,PlayButton,LabeledSlider, chips, segmented toggle.sealed Screen+rememberSaveablehost, no navigation library.JindongScaffoldconsumes status-bar, navigation-bar, and horizontal cutout insets, so the app bar clears the status bar and Dynamic Island on both platforms.ImageVectors replace emoji glyphs (play, theme toggle, chevrons, close, caret), which rendered as inconsistent color emoji on iOS. Nomaterial-iconsdependency added.How it draws the timeline
Each screen builds its pattern with the public
buildHapticPattern { }on every recomposition and maps the resulting events to bars. This path is independent of the executor, so the timeline always reflects the current control values.Known limitation: vibration replay (tracked by #84)
The actual vibration on screens 05/06/07 (and the Play replay on 01-04) currently repeats the first-compiled pattern instead of the latest one, because
rememberHapticPatternmemoizes with no keys. On screen 05 this shows up as Fade in and Fade out feeling identical; on the gallery, every card plays the same pattern. This is the bug fixed by #84.This PR does not touch that code. Each screen already passes its state as
Jindong(*keys), so once #84 lands the vibration self-heals with no further changes here. The on-screen timeline is unaffected and stays live now, which is why the harness is already useful for visual verification.Screenshots
Android
iOS
Verification
apiCheckandspotlessCheckpass; existing library tests (90 core + 40 compose) pass on Android host and iOS simulator.VibrationThreadamplitude logs).Out of scope
iOS-only parameters (
IosHapticParameters: sharpness/ADSR) are not reachable from the public DSL, so they are excluded from this harness.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes