feat(core): serial-timeline waveform pipeline (overlap, fall-ramp, handle lifecycle) - #85
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds serial haptic segment conversion and fall-ramp shaping, updates Android and iOS executors to track playback duration, and expands build wiring plus tests for overlap, gaps, ramps, expiry, and waveform execution. ChangesHaptic waveform serialization and execution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
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 |
l2hyunwoo
marked this pull request as draft
June 27, 2026 03:00
![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: 1
🧹 Nitpick comments (1)
jindong-core/src/androidHostTest/kotlin/io/github/jindong/android/AndroidVibratorTest.kt (1)
231-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlease add a virtual-time assertion for
execute().These tests lock in waveform shaping, but the PR’s user-visible fix is also the suspension length of
execute()for overlaps. A smallrunTestcase that asserts virtual time advances by the merged serial duration would keep that regression covered.🤖 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-core/src/androidHostTest/kotlin/io/github/jindong/android/AndroidVibratorTest.kt` around lines 231 - 311, Add a virtual-time assertion around HapticExecutor.execute() in AndroidVibratorTest to cover the overlap path, since the current tests only verify waveform shaping. Use the existing executor instance and a runTest block to assert that the merged-serial overlap case advances test scheduler time by the expected duration, referencing execute(), HapticPattern, and ScheduledHapticEvent so the regression in suspension length is locked in.
🤖 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/androidMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.android.kt`:
- Around line 100-113: Treat the empty merged timeline as a no-op in
HapticPattern.toWaveform(): when mergeToSerial(events) produces no segments,
short-circuit before building timings/amplitudes or calling applyDeviceCompat,
so zero-length input does not emit a compat-only waveform or trigger a spurious
buzz. Use the existing HapticPattern.toWaveform, mergeToSerial, and
applyDeviceCompat flow to add the empty-check, or alternatively enforce
durationMs > 0 at the model boundary.
---
Nitpick comments:
In
`@jindong-core/src/androidHostTest/kotlin/io/github/jindong/android/AndroidVibratorTest.kt`:
- Around line 231-311: Add a virtual-time assertion around
HapticExecutor.execute() in AndroidVibratorTest to cover the overlap path, since
the current tests only verify waveform shaping. Use the existing executor
instance and a runTest block to assert that the merged-serial overlap case
advances test scheduler time by the expected duration, referencing execute(),
HapticPattern, and ScheduledHapticEvent so the regression in suspension length
is locked in.
🪄 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: 478ba721-34c7-43fa-8548-35045204b9fa
📒 Files selected for processing (7)
gradle/libs.versions.tomljindong-core/build.gradle.ktsjindong-core/src/androidHostTest/kotlin/io/github/jindong/android/AndroidVibratorTest.ktjindong-core/src/androidMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.android.ktjindong-core/src/commonMain/kotlin/io/github/compose/jindong/core/executor/MergeToSerial.ktjindong-core/src/commonTest/kotlin/io/github/compose/jindong/core/executor/InsertFallRampsTest.ktjindong-core/src/commonTest/kotlin/io/github/compose/jindong/core/executor/MergeToSerialTest.kt
l2hyunwoo
force-pushed
the
feature/serial-timeline-merge
branch
from
June 28, 2026 08:43
e633ba5 to
084f635
Compare
l2hyunwoo
marked this pull request as ready for review
June 28, 2026 09:11
l2hyunwoo
changed the title
Contributor
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)
jindong-core/src/androidMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.android.kt (1)
165-173: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlace the Samsung primer before the first active slice.
Line 165 currently appends the
1ms off + 1ms onpair after the real waveform, so single-event patterns still start cold and instead get an extra buzz at the end. This also makesexecute()/isActivecount those extra 2ms at the wrong end of playback. Insert that pair ahead of the first non-zero amplitude instead.Suggested direction
- if (isSingleEvent) { - timings += 1L - amplitudes += 0 - timings += 1L - amplitudes += 1 - } + if (isSingleEvent) { + val firstActive = amplitudes.indexOfFirst { it != 0 } + if (firstActive >= 0) { + timings.add(firstActive, 1L) + amplitudes.add(firstActive, 0) + timings.add(firstActive + 1, 1L) + amplitudes.add(firstActive + 1, 1) + } + }🤖 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-core/src/androidMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.android.kt` around lines 165 - 173, The Samsung primer is being appended after the real waveform in HapticExecutor.android.kt, which shifts the extra 1ms off + 1ms on slice to the end instead of priming the first active event. Update the waveform-building logic in the execute() path so the primer pair is inserted before the first non-zero amplitude slice when isSingleEvent is true, and ensure any isActive / duration accounting still reflects the reordered timings.
🤖 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/androidHostTest/kotlin/io/github/jindong/android/AndroidVibratorTest.kt`:
- Around line 314-330: The zero-duration vibrator test in AndroidVibratorTest
only verifies the final idle state, so it can miss a brief compat-only trigger.
Update the `should not vibrate a zero-duration event` test to also assert that
Robolectric’s `shadowVibrator` recorded no played pattern/effect after
`executor.execute(pattern)`, in addition to `isVibrating` being false, so the
`execute` path is confirmed to be a true no-op.
---
Outside diff comments:
In
`@jindong-core/src/androidMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.android.kt`:
- Around line 165-173: The Samsung primer is being appended after the real
waveform in HapticExecutor.android.kt, which shifts the extra 1ms off + 1ms on
slice to the end instead of priming the first active event. Update the
waveform-building logic in the execute() path so the primer pair is inserted
before the first non-zero amplitude slice when isSingleEvent is true, and ensure
any isActive / duration accounting still reflects the reordered timings.
🪄 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: 699a5558-2cab-4b9d-b244-82a760716939
📒 Files selected for processing (13)
gradle/libs.versions.tomljindong-core/build.gradle.ktsjindong-core/src/androidHostTest/kotlin/io/github/compose/jindong/core/executor/AndroidHapticHandleTest.ktjindong-core/src/androidHostTest/kotlin/io/github/jindong/android/AndroidVibratorTest.ktjindong-core/src/androidMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.android.ktjindong-core/src/commonMain/kotlin/io/github/compose/jindong/core/executor/HandleExpiry.ktjindong-core/src/commonMain/kotlin/io/github/compose/jindong/core/executor/HapticHandle.ktjindong-core/src/commonMain/kotlin/io/github/compose/jindong/core/executor/HapticSegment.ktjindong-core/src/commonMain/kotlin/io/github/compose/jindong/core/executor/InsertFallRamps.ktjindong-core/src/commonMain/kotlin/io/github/compose/jindong/core/executor/MergeToSerial.ktjindong-core/src/commonTest/kotlin/io/github/compose/jindong/core/executor/HandleExpiryTest.ktjindong-core/src/commonTest/kotlin/io/github/compose/jindong/core/executor/InsertFallRampsTest.ktjindong-core/src/iosMain/kotlin/io/github/compose/jindong/core/executor/HapticExecutor.ios.kt
💤 Files with no reviewable changes (1)
- jindong-core/src/commonMain/kotlin/io/github/compose/jindong/core/executor/MergeToSerial.kt
✅ Files skipped from review due to trivial changes (1)
- jindong-core/src/commonMain/kotlin/io/github/compose/jindong/core/executor/HapticHandle.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- jindong-core/build.gradle.kts
- jindong-core/src/commonTest/kotlin/io/github/compose/jindong/core/executor/InsertFallRampsTest.kt
- gradle/libs.versions.toml
l2hyunwoo
force-pushed
the
feature/serial-timeline-merge
branch
from
June 28, 2026 11:53
121bcae to
5b21e13
Compare
wisemuji
reviewed
Jul 3, 2026
wisemuji
left a comment
wisemuji
left a comment
Contributor
There was a problem hiding this comment.
Thank you for the work!
l2hyunwoo
force-pushed
the
feature/serial-timeline-merge
branch
from
July 7, 2026 11:22
650cafd to
812098d
Compare
…on across platforms
This was referenced Jul 8, 2026
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
Reworks how the Android executor turns a
HapticPatterninto aVibrationEffect, around a new serial-timeline primitive, and fixes several correctness issues that surfaced along the way. Most of the logic now lives incommonMainso iOS shares it where applicable.The Android executor renders a pattern to a single
createWaveformcall, which can only play one amplitude at a time. The oldtoWaveformwalked events directly and broke once they overlapped (already reachable viainclude). The core change introducesmergeToSerial, which flattens overlapping events into one serial timeline, and builds the rest of the pipeline on top of it.What's included
Serial-timeline primitive (
commonMain)mergeToSerialflattens (possibly overlapping) events into non-overlappingHapticSegments. Overlaps resolve to the highest-intensity event, not a sum (amplitude clamps at 255). This is the shared building block the items below reuse.Overlap and completion-timing fixes (Android)
execute()awaits the exact played waveform length (including the primer/trailing compat segments), so the caller resumes when the vibration truly ends rather than a few ms early.Fall ramp for amplitude drops (Android, LRA only)
createWaveformis a pure step function, and a sudden drop to amplitude 0 makes an LRA ring for 50ms+.insertFallRampsborrows the front of each following gap to ease the amplitude down, preserving total duration. Gated onhasAmplitudeControl()(on ERM every non-zero amplitude rounds up to full, so a ramp would do nothing).active -> activetransitions are left untouched so intentional rhythms are not smoothed over.Handle lifecycle fix (
commonMain, both platforms)HapticHandle.isActivepreviously stayedtrueuntilcancel()because neither platform observes natural completion (the OS gives no per-effect callback). A sharedHandleExpirynow estimates completion from the expected playback length (pull-based, no coroutine/scope/timer), soisActivereflects a finished vibration. Documented as best-effort (±OS scheduling jitter);cancel()remains exact. Android and iOS fixed in lockstep.How it works
The
toWaveformpipelineWorked example:
Haptic(100ms, STRONG) + Delay(50ms) + Haptic(100ms, MEDIUM)on an LRA.STRONG= 0.75 (amplitude 191),MEDIUM= 0.5 (amplitude 127).Returns null (so nothing plays) when
mergeToSerialyields no active slice, i.e. an empty, zero-duration, or all-gap pattern. Without that guard step 4 would still append the compat tail and the motor would buzz for a pattern meant to be silent.Why
mergeToSerialresolves overlaps by max, not sumHow a fall ramp borrows from the gap (duration is conserved)
Thresholds: a gap at or below
MIN_RAMP_MS(4ms) is left whole; a gap shorter than the fullFALL_RAMP_MS(16ms) window shrinks the ramp to fit (e.g. a 10ms gap yields a 5ms ramp). The leftover gap absorbs the integer-division remainder, so the sum is conserved for any gap length (covered by a property test).Structure
toWaveformis split into the four single-purpose stages shown above. It returns null when no active slice remains.HapticSegment,mergeToSerial,insertFallRamps,HandleExpiry), matching the rest of the module.Tests
commonTest:MergeToSerialTest(merge policy, gaps, full containment, tie-break, order-independence),InsertFallRampsTest(ramp interpolation, shrink/skip thresholds, untouched transitions, plus property tests asserting duration conservation across gap parities),HandleExpiryTest(expiry boundaries with an injected time source).androidHostTest: overlap serialization, LRA ramp vs ERM gate, await duration (single-event and overlap), zero-duration no-op, sub-threshold gap, andAndroidHapticHandlewiring with injected time.Notes
relativeTimeas an absolute offset and mixes overlapping events natively. Only the shared handle-expiry fix touches iOS.internal; the.api/.klib.apidumps are unchanged.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests