fix(ui): Removed mediakit dependency from sdk#2828
Conversation
renefloor
changed the title
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughVideo playback is now replaceable through ChangesVideo player customization and default playback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StreamChat
participant ChatComponentBuilder
participant SampleAppVideoPlayer
participant DefaultStreamVideoPlayer
StreamChat->>ChatComponentBuilder: register videoPlayer builder
ChatComponentBuilder->>SampleAppVideoPlayer: pass StreamVideoPlayerProps
SampleAppVideoPlayer->>DefaultStreamVideoPlayer: use default outside supported desktop playback
SampleAppVideoPlayer-->>StreamChat: render selected video player
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@packages/stream_chat_flutter/lib/src/media_gallery_preview/video_player/stream_video_player.dart`:
- Around line 156-190: Guard the asynchronous initialization flow against
disposal: track the State’s disposed status and, after awaiting
_player.open(widget.attachment), stop before updating state or retaining the
player when disposal occurred. Ensure _player.open also rejects or exits after
its video initialization await if its own dispose path has run, preventing
ChewieController creation around a disposed VideoPlayerController; keep disposal
idempotent and avoid updates after unmounting.
- Around line 1-6: Make the video player implementation web-compatible by
isolating the dart:io-dependent File.fromUri and VideoPlayerController.file path
behind a conditional import or dart.library.io gate, removing the top-level
dart:io dependency from the widget. In the initialization flow, update the logic
after await video.initialize() to check whether the player has been disposed
before invoking _ChewieVideoPlayer.open(), and dispose or otherwise clean up the
initialized controller when teardown has already occurred.
In `@sample_app/ios/Runner.xcodeproj/project.pbxproj`:
- Around line 294-299: The iOS framework reference in the project’s framework
list is inconsistent with the declared dependency name. Update the dependency
entry in melos.yaml or regenerate the iOS project so the package and framework
use the same name, preserving the existing version constraint and generated
framework configuration.
🪄 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: 295d778d-3164-4932-8fec-ceea0f615398
📒 Files selected for processing (19)
melos.yamlpackages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/example/linux/flutter/generated_plugins.cmakepackages/stream_chat_flutter/example/windows/flutter/generated_plugins.cmakepackages/stream_chat_flutter/lib/src/components/stream_chat_component_builders.dartpackages/stream_chat_flutter/lib/src/media_gallery_preview/video_player/stream_video_player.dartpackages/stream_chat_flutter/lib/src/media_gallery_preview/video_player/stream_video_player_default.dartpackages/stream_chat_flutter/lib/src/stream_chat.dartpackages/stream_chat_flutter/lib/src/video/vlc/vlc_manager.dartpackages/stream_chat_flutter/lib/src/video/vlc/vlc_manager_desktop.dartpackages/stream_chat_flutter/lib/src/video/vlc/vlc_manager_web.dartpackages/stream_chat_flutter/lib/src/video/vlc/vlc_stub.dartpackages/stream_chat_flutter/lib/stream_chat_flutter.dartpackages/stream_chat_flutter/pubspec.yamlpackages/stream_chat_flutter/test/src/mocks.dartsample_app/ios/Runner.xcodeproj/project.pbxprojsample_app/lib/app.dartsample_app/lib/widgets/video_player.dartsample_app/pubspec.yaml
💤 Files with no reviewable changes (10)
- packages/stream_chat_flutter/example/linux/flutter/generated_plugins.cmake
- packages/stream_chat_flutter/example/windows/flutter/generated_plugins.cmake
- packages/stream_chat_flutter/lib/src/video/vlc/vlc_stub.dart
- packages/stream_chat_flutter/lib/src/video/vlc/vlc_manager.dart
- packages/stream_chat_flutter/lib/src/video/vlc/vlc_manager_desktop.dart
- packages/stream_chat_flutter/lib/src/stream_chat.dart
- packages/stream_chat_flutter/lib/src/video/vlc/vlc_manager_web.dart
- packages/stream_chat_flutter/lib/src/media_gallery_preview/video_player/stream_video_player_default.dart
- packages/stream_chat_flutter/pubspec.yaml
- packages/stream_chat_flutter/test/src/mocks.dart
| import 'dart:io'; | ||
|
|
||
| import 'package:chewie/chewie.dart'; | ||
| import 'package:flutter/widgets.dart'; | ||
| import 'package:stream_chat_flutter/src/media_gallery_preview/video_player/stream_video_player_default.dart'; | ||
| import 'package:stream_chat_flutter/src/media_gallery_preview/video_player/stream_video_player_desktop.dart'; | ||
| import 'package:stream_chat_flutter/stream_chat_flutter.dart'; | ||
| import 'package:video_player/video_player.dart'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does an unconditional import 'dart:io' in a Flutter package library fail Flutter web (dart2js/dart2wasm) compilation in current stable Flutter/Dart, or does it only throw UnimplementedError at the call site?
💡 Result:
An unconditional import of 'dart:io' in a Flutter package does not consistently cause a compilation failure for Flutter web; the behavior depends on the specific web compiler and build tooling being used [1][2]. In the current stable Flutter/Dart ecosystem: 1. Compilation Behavior: Some web compilers (such as dart2js) may allow the import to compile, while others (like those used by certain build configurations or older tooling) may reject it [1][2]. Because of this inconsistency, you cannot rely on it failing at compile time [1]. 2. Runtime Behavior: If the code compiles, attempting to access most 'dart:io' APIs at runtime will throw an UnsupportedError or similar runtime exception, as the library is not supported on the web [3][4]. 3. Recommended Practice: You should not use unconditional imports of 'dart:io' in cross-platform packages [3][5]. Instead, use conditional imports to safely include platform-specific code [6][5]. This ensures that your package remains compatible across both native and web platforms without triggering compile-time warnings or runtime errors in unsupported environments [3][5]. For official package development, use conditional imports to check for the presence of the library, as shown in this pattern [5]: import 'src/stub.dart' // Default implementation if (dart.library.io) 'src/io.dart' // dart:io implementation if (dart.library.js_interop) 'src/web.dart'; // Web implementation This approach is the standard, supported way to handle platform-specific dependencies [6][5].
Citations:
- 1: dart2js (dart compile js) allows compiling with dart:io import - runtime error dart-lang/sdk#55048
- 2: web compilers should not allow imports to 'dart:io' dart-lang/sdk#46917
- 3: https://github.com/flutter/website/blob/main/sites/docs/src/content/platform-integration/web/faq.md
- 4: Make
dart:ionot available on the web. dart-lang/sdk#47261 - 5: https://dart.dev/tools/pub/create-packages
- 6: https://gpalma.pt/blog/conditional-importing/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== target file ==\n'
cat -n packages/stream_chat_flutter/lib/src/media_gallery_preview/video_player/stream_video_player.dart | sed -n '1,280p'
printf '\n== search for conditional platform handling ==\n'
rg -n "dart:io|kIsWeb|dart.library.io|conditional import|VideoPlayerController.file|File\\(" packages/stream_chat_flutter/lib/src -SRepository: GetStream/stream-chat-flutter
Length of output: 15418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== attachment model / localUri sources ==\n'
rg -n "localUri|assetUrl" packages/stream_chat_flutter/lib packages/stream_chat_flutter/lib/src -S
printf '\n== attachment handler platform split ==\n'
cat -n packages/stream_chat_flutter/lib/src/attachment/handler/stream_attachment_handler.dart | sed -n '1,120p'
cat -n packages/stream_chat_flutter/lib/src/attachment/handler/stream_attachment_handler_base.dart | sed -n '1,260p'Repository: GetStream/stream-chat-flutter
Length of output: 7976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== usages of StreamVideoPlayer / DefaultStreamVideoPlayer ==\n'
rg -n "StreamVideoPlayer|DefaultStreamVideoPlayer|_ChewieVideoPlayer|StreamMediaGalleryPreviewItem" packages/stream_chat_flutter/lib -S
printf '\n== target preview file around video routing ==\n'
cat -n packages/stream_chat_flutter/lib/src/media_gallery_preview/stream_media_gallery_preview.dart | sed -n '120,220p'Repository: GetStream/stream-chat-flutter
Length of output: 10863
Split the IO-only path and guard teardown
- Move the
File.fromUri(...)/VideoPlayerController.file(...)branch behind a conditional import ordart.library.iogate so the web-capable widget does not depend ondart:ioat the top level. - Add a disposed check after
await video.initialize().dispose()can run while initialization is in flight, which lets_ChewieVideoPlayer.open()create a controller after teardown and leak the pair.
🤖 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
`@packages/stream_chat_flutter/lib/src/media_gallery_preview/video_player/stream_video_player.dart`
around lines 1 - 6, Make the video player implementation web-compatible by
isolating the dart:io-dependent File.fromUri and VideoPlayerController.file path
behind a conditional import or dart.library.io gate, removing the top-level
dart:io dependency from the widget. In the initialization flow, update the logic
after await video.initialize() to check whether the player has been disposed
before invoking _ChewieVideoPlayer.open(), and dispose or otherwise clean up the
initialized controller when teardown has already occurred.
| Attachment get attachment => props.attachment; | ||
|
|
||
| /// The 0-based index of this page in the enclosing | ||
| /// [StreamMediaGalleryPreview]. | ||
| int get pageIndex => props.pageIndex; | ||
|
|
||
| /// Whether playback should auto-start when this page first becomes | ||
| /// active. | ||
| bool get autoplay => props.autoplay; |
There was a problem hiding this comment.
I personally would just remove these getters to be similar to other components in the SDK
There was a problem hiding this comment.
This way the change is smaller for the rest of the code and also maybe somebody used StreamVideoPlayer.attachment somewhere for some reason. This is just safer and smaller change.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2828 +/- ##
==========================================
+ Coverage 71.35% 71.48% +0.13%
==========================================
Files 430 426 -4
Lines 26945 26892 -53
==========================================
- Hits 19226 19224 -2
+ Misses 7719 7668 -51 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Submit a pull request
Linear: FLU-615
Github Issue: #2768
CLA
Description of the pull request
This PR removes the media_kit dependency in the stream_chat_flutter sdk. It's currently only used to play videos on Windows and Linux desktop platforms. For these platforms we used update the docs with a sample how to implement it yourselves as also done in the sample app.
Screenshots / Videos
Summary by CodeRabbit
New Features
Bug Fixes
Chores