Conversation
tddang-linagora
requested review from
9clg6,
dab246,
hoangdat and
nqhhdev
as code owners
WalkthroughThe PR adds URL-detection to MessageContentBuilderMixin._checkNeedAddNewLine so a found URL forces a newline; removes the popup-menu Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lib/widgets/twake_components/twake_preview_link/twake_link_preview_item.dart`:
- Around line 78-88: The outer InkWell in TwakeLinkPreviewItem is duplicating
the tap handler already provided inside LinkPreviewBuilder, causing nested tap
events; remove the outer InkWell wrapper (the InkWell in TwakeLinkPreviewItem
that calls UrlLauncher(context, url: widget.previewLink).launchUrl()) so only
the InkWell inside LinkPreviewBuilder handles taps, or alternatively remove the
InkWell inside LinkPreviewBuilder and keep the outer one—choose one consistent
place for the onTap (reference: TwakeLinkPreviewItem, LinkPreviewBuilder,
UrlLauncher.launchUrl, previewLink).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4a3b5fc-2020-4718-a1c0-6af58e0dbbf8
📒 Files selected for processing (7)
lib/pages/chat/events/message/message_content_builder_mixin.dartlib/presentation/mixins/linkify_mixin.dartlib/widgets/mixins/get_preview_url_mixin.dartlib/widgets/twake_components/twake_preview_link/twake_link_preview.dartlib/widgets/twake_components/twake_preview_link/twake_link_preview_item.darttest/pages/chat/events/message/message_content_builder_mixin_test.darttest/widget/message/twake_link_preview_item_test.dart
💤 Files with no reviewable changes (1)
- lib/presentation/mixins/linkify_mixin.dart
lib/widgets/twake_components/twake_preview_link/twake_link_preview_item.dart
Outdated
Show resolved
Hide resolved
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2956 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lib/widgets/twake_components/twake_preview_link/twake_link_preview_item.dart`:
- Around line 32-38: When previewLink becomes null/empty or Uri.tryParse returns
null, the widget currently returns early and leaves the previous preview/state
and any in-flight subscription active; update _getPreviewInfo() to call the
mixin reset routine instead of returning—specifically invoke
clearPreviewUrlState() (which cancels _getPreviewUrlSubscription and sets
getPreviewUrlStateNotifier to Right(GetPreviewUrlInitial())) before returning,
and do the same for the other early-return branch that handles invalid links so
any previous preview is cleared and in-flight requests are cancelled before
exiting; ensure you reference _getPreviewInfo(), previewLink, getPreviewUrl(),
clearPreviewUrlState(), _getPreviewUrlSubscription, getPreviewUrlStateNotifier
and GetPreviewUrlInitial() when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb53c8eb-d000-437c-aeee-bfc496cb6e13
📒 Files selected for processing (2)
lib/widgets/twake_components/twake_preview_link/twake_link_preview_item.darttest/pages/chat/events/message/message_content_builder_mixin_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- test/pages/chat/events/message/message_content_builder_mixin_test.dart
lib/widgets/twake_components/twake_preview_link/twake_link_preview_item.dart
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
LGTM |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/widgets/twake_components/twake_preview_link/twake_link_preview_item.dart (1)
68-81: Verify color consistency between skeleton and success states.The container background colors differ between states:
- Success:
primaryContainer/onSurface.withOpacity(0.08)- Skeleton:
primary[95]/LinagoraStateLayer(...).opacityLayer1This may cause a visual flicker when transitioning from loading to loaded. Consider using the same colors for both states, or confirm this difference is intentional.
Also applies to: 92-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/widgets/twake_components/twake_preview_link/twake_link_preview_item.dart` around lines 68 - 81, The success and skeleton background colors in TwakeLinkPreviewItem differ and cause flicker; update the skeleton rendering (the block around lines 92-107) and the main Container (the block using TwakeLinkPreviewItem.linkPreviewBodyKey and TwakeLinkPreviewItemStyle) to reuse a single background color expression based on widget.ownMessage (e.g., compute a local backgroundColor variable once and use it in both the ShapeDecoration and the skeleton LinagoraStateLayer/primary[95] usage) so both loading and loaded states share the same color logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/widgets/mixins/get_preview_url_mixin.dart`:
- Around line 23-28: Dispose currently cancels _getPreviewUrlSubscription after
disposing getPreviewUrlStateNotifier which allows a stream callback
(_handleGetPreviewUrlOnData) to run and touch a disposed ValueNotifier; swap the
order so _getPreviewUrlSubscription?.cancel() is called before
getPreviewUrlStateNotifier.dispose() inside the dispose() override to prevent
callbacks from writing to a disposed notifier (refer to dispose(),
_getPreviewUrlSubscription, getPreviewUrlStateNotifier, and
_handleGetPreviewUrlOnData).
---
Nitpick comments:
In
`@lib/widgets/twake_components/twake_preview_link/twake_link_preview_item.dart`:
- Around line 68-81: The success and skeleton background colors in
TwakeLinkPreviewItem differ and cause flicker; update the skeleton rendering
(the block around lines 92-107) and the main Container (the block using
TwakeLinkPreviewItem.linkPreviewBodyKey and TwakeLinkPreviewItemStyle) to reuse
a single background color expression based on widget.ownMessage (e.g., compute a
local backgroundColor variable once and use it in both the ShapeDecoration and
the skeleton LinagoraStateLayer/primary[95] usage) so both loading and loaded
states share the same color logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b33795e-ca2f-42f8-bfe5-ab6f343c7db7
📒 Files selected for processing (2)
lib/widgets/mixins/get_preview_url_mixin.dartlib/widgets/twake_components/twake_preview_link/twake_link_preview_item.dart
| @override | ||
| void dispose() { | ||
| getPreviewUrlStateNotifier.dispose(); | ||
| _getPreviewUrlSubscription?.cancel(); | ||
| super.dispose(); | ||
| } |
There was a problem hiding this comment.
Cancel subscription before disposing the notifier.
There's a subtle timing issue: if a stream event is delivered between disposing the notifier (line 25) and canceling the subscription (line 26), _handleGetPreviewUrlOnData would attempt to set a value on a disposed ValueNotifier, potentially throwing an error.
🔧 Proposed fix: Swap the order
`@override`
void dispose() {
- getPreviewUrlStateNotifier.dispose();
_getPreviewUrlSubscription?.cancel();
+ getPreviewUrlStateNotifier.dispose();
super.dispose();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @override | |
| void dispose() { | |
| getPreviewUrlStateNotifier.dispose(); | |
| _getPreviewUrlSubscription?.cancel(); | |
| super.dispose(); | |
| } | |
| `@override` | |
| void dispose() { | |
| _getPreviewUrlSubscription?.cancel(); | |
| getPreviewUrlStateNotifier.dispose(); | |
| super.dispose(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/widgets/mixins/get_preview_url_mixin.dart` around lines 23 - 28, Dispose
currently cancels _getPreviewUrlSubscription after disposing
getPreviewUrlStateNotifier which allows a stream callback
(_handleGetPreviewUrlOnData) to run and touch a disposed ValueNotifier; swap the
order so _getPreviewUrlSubscription?.cancel() is called before
getPreviewUrlStateNotifier.dispose() inside the dispose() override to prevent
callbacks from writing to a disposed notifier (refer to dispose(),
_getPreviewUrlSubscription, getPreviewUrlStateNotifier, and
_handleGetPreviewUrlOnData).
3 participants
Ticket
Reproduce
reproduce.mp4
Resolved
resolved.mp4
Summary by CodeRabbit
Bug Fixes
Refactor
Tests