WebRTC Client. Remove redundant targets. #5167
Conversation
WalkthroughThe PR removes JVM WebRTC publications and test utilities while expanding the multiplatform API surface for Android and iOS. The JVM public API is entirely deleted, and build configuration disables desktop/JVM targets in favor of androidNative. Desktop and JVM test utility files are removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
gradle/artifacts/publishAndroidNativePublications.txt(1 hunks)gradle/artifacts/publishDarwinPublications.txt(1 hunks)gradle/artifacts/publishJsPublications.txt(1 hunks)gradle/artifacts/publishJvmAndCommonPublications.txt(1 hunks)ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api(0 hunks)ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api(12 hunks)ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api(14 hunks)ktor-client/ktor-client-webrtc/desktop/test/io/ktor/client/webrtc/utils/ConnectionUtils.desktop.kt(0 hunks)ktor-client/ktor-client-webrtc/desktop/test/io/ktor/client/webrtc/utils/Ignore.desktop.kt(0 hunks)ktor-client/ktor-client-webrtc/gradle.properties(1 hunks)ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt(0 hunks)ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt(0 hunks)
💤 Files with no reviewable changes (5)
- ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt
- ktor-client/ktor-client-webrtc/desktop/test/io/ktor/client/webrtc/utils/Ignore.desktop.kt
- ktor-client/ktor-client-webrtc/desktop/test/io/ktor/client/webrtc/utils/ConnectionUtils.desktop.kt
- ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
- ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api
🧰 Additional context used
📓 Path-based instructions (1)
**/api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Track all public API changes in the /api/ directories
Files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.apiktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
🧠 Learnings (18)
📓 Common learnings
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/connection.rs:222-236
Timestamp: 2025-08-15T08:43:28.980Z
Learning: In WebRTC.rs (webrtc crate), remote track removal detection is limited compared to browser WebRTC APIs. The onmute event is the most reliable indicator for track removal, as transceiver direction changes after renegotiation don't happen immediately and have asynchronous timing issues. Polling solutions are not recommended due to performance concerns.
📚 Learning: 2025-04-22T12:33:16.705Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4805
File: build-logic/src/main/kotlin/ktorbuild/internal/publish/ValidatePublishedArtifactsTask.kt:74-82
Timestamp: 2025-04-22T12:33:16.705Z
Learning: The ValidatePublishedArtifactsTask in the Ktor build system is designed to collect artifacts from all PublishToMavenRepository tasks in the task graph, not just from the specifically named publish task. This is intentional to validate all publications triggered by a task, including those from task dependencies.
Applied to files:
gradle/artifacts/publishJvmAndCommonPublications.txtgradle/artifacts/publishAndroidNativePublications.txtgradle/artifacts/publishDarwinPublications.txtgradle/artifacts/publishJsPublications.txt
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Applies to **/*.kt : Document all public Kotlin APIs, including parameters, return types, and exceptions
Applied to files:
gradle/artifacts/publishJvmAndCommonPublications.txtgradle/artifacts/publishAndroidNativePublications.txtgradle/artifacts/publishJsPublications.txtktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module depends on the kotlin("plugin.atomicfu") plugin, so atomicfu should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
gradle/artifacts/publishJvmAndCommonPublications.txtgradle/artifacts/publishAndroidNativePublications.txtgradle/artifacts/publishDarwinPublications.txtktor-client/ktor-client-webrtc/gradle.propertiesgradle/artifacts/publishJsPublications.txt
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Applies to **/*.{kt,kts} : Use star imports for io.ktor.* packages
Applied to files:
gradle/artifacts/publishJvmAndCommonPublications.txtgradle/artifacts/publishAndroidNativePublications.txtgradle/artifacts/publishDarwinPublications.txtgradle/artifacts/publishJsPublications.txt
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Gobley Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module requires the kotlin("plugin.atomicfu") plugin as a dependency, so the atomicfu plugin should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
gradle/artifacts/publishJvmAndCommonPublications.txtgradle/artifacts/publishAndroidNativePublications.txtgradle/artifacts/publishDarwinPublications.txtgradle/artifacts/publishJsPublications.txt
📚 Learning: 2025-05-07T09:12:14.293Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4836
File: ktor-utils/build.gradle.kts:35-35
Timestamp: 2025-05-07T09:12:14.293Z
Learning: The Ktor project maintains a flat Gradle project structure (where projects are referenced without nested paths like `:ktor-test-base`) while keeping a hierarchical directory organization on disk.
Applied to files:
gradle/artifacts/publishJvmAndCommonPublications.txtgradle/artifacts/publishAndroidNativePublications.txt
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Project is Kotlin Multiplatform (JVM, JS, Native)
Applied to files:
gradle/artifacts/publishAndroidNativePublications.txtgradle/artifacts/publishDarwinPublications.txtktor-client/ktor-client-webrtc/gradle.propertiesgradle/artifacts/publishJsPublications.txtktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Binary compatibility is enforced via Kotlin Gradle Plugin ABI validation
Applied to files:
gradle/artifacts/publishAndroidNativePublications.txt
📚 Learning: 2025-07-01T10:54:53.751Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4970
File: build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts:67-72
Timestamp: 2025-07-01T10:54:53.751Z
Learning: In build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts, the KT-70915 and KT-77609 workarounds that limit parallelism for KotlinNativeLink and CInteropCommonizerTask should remain unconditional (not gated behind CI flag) as these limits are beneficial for both local and CI builds.
Applied to files:
ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: This is a multiplatform project (JVM, JS, Native), and platform-specific considerations should be respected.
Applied to files:
ktor-client/ktor-client-webrtc/gradle.properties
📚 Learning: 2025-05-16T13:11:28.416Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4860
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.api:24-26
Timestamp: 2025-05-16T13:11:28.416Z
Learning: Binary incompatible changes (such as constructor signature changes) are acceptable in the ktor-server-di module when the version is not yet released, as confirmed by the development team.
Applied to files:
gradle/artifacts/publishJsPublications.txtktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-08-15T09:12:47.243Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/senders.rs:88-96
Timestamp: 2025-08-15T09:12:47.243Z
Learning: In the webrtc Rust crate version used in the Ktor WebRTC RS implementation, the RTCRtpCodingParameters struct's rtx field is NOT optional and can be accessed directly as params.rtx.ssrc without needing Option handling.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.apiktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-06-09T07:08:35.085Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4916
File: ktor-server/ktor-server-core/api/ktor-server-core.api:151-151
Timestamp: 2025-06-09T07:08:35.085Z
Learning: Breaking changes are acceptable in Ktor codebase when the code hasn't been released yet, as confirmed by bjhham from the development team.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-08-15T08:43:28.980Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/connection.rs:222-236
Timestamp: 2025-08-15T08:43:28.980Z
Learning: In WebRTC.rs (webrtc crate), remote track removal detection is limited compared to browser WebRTC APIs. The onmute event is the most reliable indicator for track removal, as transceiver direction changes after renegotiation don't happen immediately and have asynchronous timing issues. Polling solutions are not recommended due to performance concerns.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-10-13T14:43:54.732Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5122
File: ktor-client/ktor-client-webrtc/jvm/src/io/ktor/client/webrtc/JvmWebRtcDataChannel.kt:70-72
Timestamp: 2025-10-13T14:43:54.732Z
Learning: The dev.onvoid.webrtc.RTCDataChannel library (webrtc-java) does not provide a native setBufferedAmountLowThreshold method. The threshold must be emulated by storing it locally and manually checking the buffered amount in the onBufferedAmountChange callback.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: Jetty's AbstractConnection class implements Closeable interface, so any class extending AbstractConnection (like JettyWebsocketConnection) can be used with Kotlin's use { } extension function for resource management.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
🔇 Additional comments (12)
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api (6)
2-3: LGTM! Clear target specification aligns with PR objectives.The explicit target list now only includes platforms with WebRTC client implementations (Android Native, iOS, JS, WASM), removing previously published but unsupported targets like JVM, desktop native, and tvOS. This change improves clarity for users and prevents confusion about platform support.
Note: This is a breaking change for users who were depending on the library for removed targets.
As per coding guidelines: This change should be tracked in the /api/ directory.
835-835: New method completes the Capturer API.The addition of
startCapture()completes the capture lifecycle API alongside the existingstopCapture()andisCapturingproperty. This is a logical and consistent extension of the public interface for iOS video capture functionality.As per coding guidelines: Public API addition tracked in /api/ directory.
891-892: LGTM! Native factory exposure enables platform-specific customization.The
peerConnectionFactoryproperty exposes the underlying nativeRTCPeerConnectionFactory, enabling advanced users to access iOS-specific WebRTC functionality while maintaining type safety. This follows the same pattern as thegetNative()extension functions added elsewhere in the API.As per coding guidelines: Public API addition tracked in /api/ directory.
917-920: LGTM! Default constants complete the simulator capture configuration.The addition of
DEFAULT_HEIGHTandDEFAULT_WIDTHconstants complements the existingDEFAULT_FPS, providing a complete set of default values for simulator video capture configuration. This improves developer experience by offering sensible defaults for common use cases.As per coding guidelines: Public API addition tracked in /api/ directory.
1043-1045: LGTM! Custom factory configuration enhances flexibility.The
rtcFactoryproperty allows users to provide a customRTCPeerConnectionFactoryfor advanced iOS WebRTC configuration scenarios. The nullable, mutable design provides flexibility while maintaining backward compatibility with a default factory implementation.As per coding guidelines: Public API addition tracked in /api/ directory.
1053-1072: LGTM! Native interop functions follow consistent pattern.The
getNative()extension functions provide escape hatches for accessing platform-specific native types across multiple WebRTC abstractions. This pattern:
- Enables advanced platform-specific functionality while maintaining the multiplatform abstraction
- Follows a consistent naming convention across all exposed types
- Mirrors similar functionality provided for JS/WASM targets (lines 1235-1250)
- Represents non-breaking additions to the public API
As per coding guidelines: Public API additions tracked in /api/ directory.
gradle/artifacts/publishDarwinPublications.txt (1)
843-857: Code changes verified correct; iOS-only WebRTC implementation confirmed.The review comment's assertions are accurate:
- Lines 843-857 contain iOS WebRTC entries (iosarm64, iossimulatorarm64, iosx64) with all artifact types including cinterop-WebRTC.klib
- tvOS, watchOS, and macOS WebRTC entries are absent throughout the file (verified by pattern search)
- File contains exactly 15 WebRTC entries, all iOS-only
No issues found.
gradle/artifacts/publishAndroidNativePublications.txt (1)
157-168: AndroidNative WebRTC publications are correctly configured and generated.The ktor-client-webrtc module is properly included in settings.gradle.kts and has an androidNative source set. KtorTargets.kt correctly defines all four androidNative variants (arm32/arm64/x64/x86), and the publications in gradle/artifacts/publishAndroidNativePublications.txt match the expected output: 4 targets × 3 artifact types (klib, javadoc, sources).
gradle/artifacts/publishJsPublications.txt (1)
85-90: JS/WebAssembly publications align with NodeJS disabled setup.Verified: no Node.js artifacts present, only browser/wasm-js variants (lines 85–90) with correct artifact types (.klib, javadoc.jar, sources.jar).
gradle/artifacts/publishJvmAndCommonPublications.txt (1)
179-185: Android AAR + common publication verified; JVM WebRTC properly removed.Verification confirms:
- No
ktor-client-webrtc-jvmartifact in publications ✓- Android configured via
com.android.kotlin.multiplatform.libraryplugin with proper AAR publication ✓- Common multiplatform JAR + metadata present ✓
- No stray JVM platform references in WebRTC modules ✓
Code changes are correct.
ktor-client/ktor-client-webrtc/gradle.properties (1)
7-11: gradle.properties target configuration is correct—no changes needed.The verification confirms that the target matrix is properly configured:
target.android=trueandtarget.ios=trueenable WebRTC for Android and iOStarget.darwin=falsedisables the darwin group (macOS/tvOS/watchOS), buttarget.ios=trueoverrides this at the module leveltarget.jvm=falseandtarget.desktop=falsedisable JVM and desktop builds- The build-logic correctly reads these flags via
ProjectGradlePropertiesandKtorTargets.isEnabled(), controlling which targets are added to the Kotlin multiplatform configuration- The webrtc module has no sourcesets for disabled targets, so only enabled targets can produce artifacts
- No stray publications for jvm, desktop, tvos, watchos, or macos will occur
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api (1)
74-76: Avoid breaking the Android peer connection constructor.
The new mandatoryFunction1parameter onAndroidWebRtcPeerConnectionreplaces the former two-argument constructor, so any client code instantiating this public type now fails to compile/link. Please keep the old signature available (e.g., via an overload delegating to the new logic) so the “remove redundant targets” work doesn’t introduce an unrelated ABI/source break.⛔ Skipped due to learnings
Learnt from: bjhham Repo: ktorio/ktor PR: 4855 File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336 Timestamp: 2025-05-14T18:05:02.321Z Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.Learnt from: osipxd Repo: ktorio/ktor PR: 4860 File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.api:24-26 Timestamp: 2025-05-16T13:11:28.416Z Learning: Binary incompatible changes (such as constructor signature changes) are acceptable in the ktor-server-di module when the version is not yet released, as confirmed by the development team.Learnt from: bjhham Repo: ktorio/ktor PR: 4916 File: ktor-server/ktor-server-core/api/ktor-server-core.api:151-151 Timestamp: 2025-06-09T07:08:35.085Z Learning: Breaking changes are acceptable in Ktor codebase when the code hasn't been released yet, as confirmed by bjhham from the development team.
| public abstract interface class io/ktor/client/webrtc/WebRtcConnectionEvents { | ||
| public abstract fun getDataChannelEvents ()Lkotlinx/coroutines/flow/SharedFlow; | ||
| public abstract fun getIceCandidates ()Lkotlinx/coroutines/flow/SharedFlow; |
There was a problem hiding this comment.
Maintain compatibility for WebRtcConnectionEvents.
Adding getDataChannelEvents() as a new abstract member forces every existing implementation of this public interface to change, which is a hard binary break. Consider providing a default implementation (or an opt-in extension) instead of making it mandatory so users extending the interface don’t break when upgrading.
@osipxd Should I manually merge this branch to |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-io/api/ktor-io.api(1 hunks)ktor-server/ktor-server-core/api/ktor-server-core.api(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ktor-server/ktor-server-core/api/ktor-server-core.api
🧰 Additional context used
📓 Path-based instructions (1)
**/api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Track all public API changes in the /api/ directories
Files:
ktor-io/api/ktor-io.api
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Project is Kotlin Multiplatform (JVM, JS, Native)
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: This is a multiplatform project (JVM, JS, Native), and platform-specific considerations should be respected.
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: Jetty's AbstractConnection class implements Closeable interface, so any class extending AbstractConnection (like JettyWebsocketConnection) can be used with Kotlin's use { } extension function for resource management.
Applied to files:
ktor-io/api/ktor-io.api
📚 Learning: 2025-10-22T07:21:51.263Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 5139
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyInjection.kt:11-11
Timestamp: 2025-10-22T07:21:51.263Z
Learning: In Ktor, `io.ktor.utils.io.CancellationException` is a typealias for `kotlinx.coroutines.CancellationException`, which is a supertype of `JobCancellationException`. Therefore, checking `e !is io.ktor.utils.io.CancellationException` is sufficient to exclude all coroutine cancellation exceptions.
Applied to files:
ktor-io/api/ktor-io.api
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Applies to **/*.kt : Follow Kotlin error-handling conventions and use specific Ktor exceptions
Applied to files:
ktor-io/api/ktor-io.api
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: JettyWebsocketConnection in ktor-server-jetty-jakarta implements Closeable interface (has close() method), so connection.use { } is valid Kotlin syntax and will compile correctly.
Applied to files:
ktor-io/api/ktor-io.api
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.
Applied to files:
ktor-io/api/ktor-io.api
Main will be merged into the release branch only after 3.4.0 release. So let's cherry-pick this change manually to include it in 3.3.x patch releases. |
zibet27
force-pushed
the
zibet27/webrtc-remove-redundant-targets
branch
from
cd9657a to
ae8521e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api (1)
554-555: Binary-incompatible change to public interface.Adding the abstract method
getDataChannelEvents()toWebRtcConnectionEventsis a breaking change that forces all existing implementations to be updated. This was already flagged in previous reviews.Based on learnings, breaking changes may be acceptable in Ktor when code hasn't been released yet. However, if this module has been released, consider providing a default implementation to maintain binary compatibility.
🧹 Nitpick comments (3)
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api (1)
891-892: Consider documenting platform-specific native access as advanced API.The
peerConnectionFactoryproperty exposes the native iOSRTCPeerConnectionFactorytype, providing an escape hatch for advanced use cases. While this is useful for interoperability, it creates a platform-specific dependency.Consider adding KDoc documentation indicating this is an advanced API for users who need direct access to the underlying platform framework.
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api (2)
136-144: Consider whether DefaultExceptionHandler should be public API.
DefaultExceptionHandlerappears to be infrastructure for internal coroutine exception handling. Exposing this as public API may leak implementation details and create maintenance burden. Unless users need to extend or reference this class directly, consider marking it as internal.
679-686: Constraint classes changed from immutable to mutable.Adding setters to
AudioTrackConstraintsandVideoTrackConstraintschanges these from immutable to mutable. While this enables DSL-style configuration, it introduces potential concerns:
- Shared mutable state issues
- Thread-safety considerations
- Less predictable behavior compared to immutable data classes
Consider whether a builder pattern or immutable approach with copy methods would better serve the API design, especially in a concurrent environment like WebRTC.
Also applies to: 755-760
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
gradle/artifacts/publishJvmAndCommonPublications.txt(0 hunks)gradle/artifacts/publishLinuxPublications.txt(0 hunks)gradle/artifacts/publishWindowsPublications.txt(0 hunks)ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api(0 hunks)ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api(12 hunks)ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api(14 hunks)ktor-client/ktor-client-webrtc/desktop/test/io/ktor/client/webrtc/utils/ConnectionUtils.desktop.kt(0 hunks)ktor-client/ktor-client-webrtc/desktop/test/io/ktor/client/webrtc/utils/Ignore.desktop.kt(0 hunks)ktor-client/ktor-client-webrtc/gradle.properties(1 hunks)ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt(0 hunks)ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt(0 hunks)
💤 Files with no reviewable changes (8)
- ktor-client/ktor-client-webrtc/desktop/test/io/ktor/client/webrtc/utils/Ignore.desktop.kt
- gradle/artifacts/publishWindowsPublications.txt
- gradle/artifacts/publishJvmAndCommonPublications.txt
- gradle/artifacts/publishLinuxPublications.txt
- ktor-client/ktor-client-webrtc/desktop/test/io/ktor/client/webrtc/utils/ConnectionUtils.desktop.kt
- ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/ConnectionUtils.jvm.kt
- ktor-client/ktor-client-webrtc/jvm/test/io/ktor/client/webrtc/utils/Ignore.jvm.kt
- ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-client/ktor-client-webrtc/gradle.properties
🧰 Additional context used
📓 Path-based instructions (1)
**/api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Track all public API changes in the /api/ directories
Files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.apiktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: This is a multiplatform project (JVM, JS, Native), and platform-specific considerations should be respected.
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Project is Kotlin Multiplatform (JVM, JS, Native)
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Start with JVM-only implementation and tests unless requested otherwise
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Project is Kotlin Multiplatform (JVM, JS, Native)
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api
📚 Learning: 2025-08-15T09:12:47.243Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/senders.rs:88-96
Timestamp: 2025-08-15T09:12:47.243Z
Learning: In the webrtc Rust crate version used in the Ktor WebRTC RS implementation, the RTCRtpCodingParameters struct's rtx field is NOT optional and can be accessed directly as params.rtx.ssrc without needing Option handling.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.apiktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Applies to **/*.kt : Document all public Kotlin APIs, including parameters, return types, and exceptions
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-05-16T13:11:28.416Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4860
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.api:24-26
Timestamp: 2025-05-16T13:11:28.416Z
Learning: Binary incompatible changes (such as constructor signature changes) are acceptable in the ktor-server-di module when the version is not yet released, as confirmed by the development team.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-06-09T07:08:35.085Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4916
File: ktor-server/ktor-server-core/api/ktor-server-core.api:151-151
Timestamp: 2025-06-09T07:08:35.085Z
Learning: Breaking changes are acceptable in Ktor codebase when the code hasn't been released yet, as confirmed by bjhham from the development team.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-08-15T08:43:28.980Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/connection.rs:222-236
Timestamp: 2025-08-15T08:43:28.980Z
Learning: In WebRTC.rs (webrtc crate), remote track removal detection is limited compared to browser WebRTC APIs. The onmute event is the most reliable indicator for track removal, as transceiver direction changes after renegotiation don't happen immediately and have asynchronous timing issues. Polling solutions are not recommended due to performance concerns.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-10-13T14:43:54.732Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5122
File: ktor-client/ktor-client-webrtc/jvm/src/io/ktor/client/webrtc/JvmWebRtcDataChannel.kt:70-72
Timestamp: 2025-10-13T14:43:54.732Z
Learning: The dev.onvoid.webrtc.RTCDataChannel library (webrtc-java) does not provide a native setBufferedAmountLowThreshold method. The threshold must be emulated by storing it locally and manually checking the buffered amount in the onBufferedAmountChange callback.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: Jetty's AbstractConnection class implements Closeable interface, so any class extending AbstractConnection (like JettyWebsocketConnection) can be used with Kotlin's use { } extension function for resource management.
Applied to files:
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api
🔇 Additional comments (15)
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api (4)
2-3: Target list cleanup aligns with PR objectives.The removal of JVM, desktop, and tvOS targets appropriately addresses the PR's motivation to eliminate redundant targets that lack implementations. The explicit iOS alias improves clarity over generic "apple" target naming.
Note: This is a breaking change for any users currently depending on the removed targets. Ensure migration guidance is provided in release notes.
835-835: Addition ofstartCapture()completes the Capturer interface API.Previously, the
Capturerinterface hadstopCapture()but nostartCapture(), creating an asymmetric API. This addition provides a complete lifecycle API for video capture operations.
917-920: DEFAULT_HEIGHT and DEFAULT_WIDTH constants complete the default parameters set.Adding these constants alongside the existing DEFAULT_FPS provides a complete set of default video parameters for the simulator capturer.
Minor observation:
CameraVideoCapturer.Companion.DEFAULT_FPSis typed asDouble(line 876), whileSimulatorVideoCapturer.Companion.DEFAULT_FPSis typed asInt(line 916). This type difference may be intentional but could create friction if users switch between capturers.
1043-1075: Well-designed escape hatch API for iOS platform interoperability.The additions provide a consistent pattern for accessing underlying native iOS WebRTC objects:
rtcFactoryproperty (lines 1043-1045) enables custom factory injectiongetNative()extension functions (lines 1054-1072) provide uniform access to native objects across all major WebRTC typesdefaultVideoCapturerFactory()(line 1075) offers sensible default behaviorThis pattern follows Kotlin Multiplatform best practices for platform-specific APIs, maintaining consistency while providing necessary flexibility for advanced use cases.
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.api (11)
35-52: Significant API expansion for Android data channels.This adds comprehensive data channel support for Android, including lifecycle management, buffering control, and message sending. The API surface follows WebRTC standards.
211-280: Well-designed data channel API.The
WebRtc$DataChannelinterface and supporting types provide a clean, type-safe API for WebRTC data channels. The message abstraction with Binary/Text variants and the state management withcanSend()helper follow good design practices.
533-534: Good configuration options for data channel events.Adding
dataChannelEventsReplayandexceptionHandlertoWebRtcConnectionConfigprovides appropriate control over data channel event buffering and error handling behavior.Also applies to: 543-544
566-587: New public events emitter implementation.
WebRtcConnectionEventsEmitterprovides the concrete implementation for emitting WebRTC connection events. Exposing this as public API may be intentional for testing or custom implementations, but consider whether this should be internal infrastructure.
589-600: Base class for data channel implementations.
WebRtcDataChannelprovides a solid foundation for platform-specific data channel implementations. The mangled method nameemitMessage-JP2dKIUis expected for Kotlin inline value classes or similar language features.
764-793: Peer connection API extended with data channel support.The additions to
WebRtcPeerConnectionproperly integrate data channel functionality:
createDataChannel()with DSL-style configurationawaitIceGatheringComplete()utility for ICE gathering coordination- Protected accessors for coroutine scope and events emitter
The API design maintains consistency with existing patterns.
314-322: Standard serialization interface update.The addition of
typeParametersSerializers()methods to the serializers is part of kotlinx.serialization'sGeneratedSerializerinterface contract. This is standard Kotlin serialization infrastructure.Also applies to: 440-448
58-61: Excellent usability improvement with DSL-style overloads.The addition of
Function1-based overloads for track and peer connection creation enables clean DSL-style configuration:createAudioTrack { autoGainControl = true echoCancellation = true }This pattern is consistently applied across
MediaTrackFactory,WebRtcEngine,WebRtcClient, and platform implementations. These are non-breaking additions that significantly improve API ergonomics.Also applies to: 148-152, 503-507, 805-807
801-801: Useful constant for default frame rate.Adding
DEFAULT_FRAME_RATEas a public constant provides a clear reference value for video track configuration on Android.
831-832: Improved type safety for native track access.The type-specific
getNative()overloads forAudioTrackandVideoTrackprovide better type safety by returning the specific native types rather than a genericMediaStreamTrack.
1-834: API file correctly reflects platform consolidation—verify KDoc documentation.Verification confirms that JVM/desktop/tvOS targets were legitimately removed from the module. The
build.gradle.ktsdeclares only Android, iOS, JavaScript, and WebAssembly targets with no remnants of removed platforms. The two API files (api/android/andapi/ktor-client-webrtc.api) correctly represent the new multiplatform surface.The extensive API additions (lines 1-834) are valid expansions for the remaining platforms, primarily adding comprehensive data channel support. The original concern about disconnect between PR description and file contents is resolved: the file shows expansions because it now represents the consolidated API for all remaining targets.
Per the coding guidelines for Kotlin APIs, ensure all public APIs including parameters, return types, and exceptions are documented. Verify that newly added public classes and interfaces—particularly
DataChannelEvent,WebRtcConnectionEventsEmitter, and constraint classes—include appropriate KDoc comments.
Subsystem
WebRTC Client.
Motivation
We have some targets like jvm, desktopNative, tvos, etc that are published but don't have clients, which is confusing for users.
Solution
Remove those targets.