Conversation
9clg6
requested review from
dab246,
hoangdat,
nqhhdev and
tddang-linagora
as code owners
WalkthroughThe changes optimize chat message rendering and timeline memory management. A Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2933 |
9clg6
force-pushed
the
perf/repaint-boundary-messages
branch
from
3d86348 to
01ae931
Compare
tddang-linagora
left a comment
tddang-linagora
left a comment
There was a problem hiding this comment.
- Add before after screenshots of the improvement
|
9clg6
force-pushed
the
perf/repaint-boundary-messages
branch
from
01ae931 to
cbe83f2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/pages/chat/chat_event_list_item.dart (1)
63-79: Avoid callinglistHorizontalActionMenuBuildertwice in one build.Line 63 and Line 75 invoke the same builder for the same
event. Cache once and reuse to reduce build-time overhead and keep both menu representations consistent.♻️ Proposed refactor
`@override` Widget build(BuildContext context) { if (!event.isVisibleInGui) return const SizedBox(); + final horizontalActions = controller.listHorizontalActionMenuBuilder(event); return AutoScrollTag( key: ValueKey(event.eventId), @@ - listHorizontalActionMenu: controller.listHorizontalActionMenuBuilder( - event, - ), + listHorizontalActionMenu: horizontalActions, @@ - listAction: controller.listHorizontalActionMenuBuilder(event).map(( + listAction: horizontalActions.map(( action, ) { return ContextMenuAction(name: action.action.name); }).toList(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/chat/chat_event_list_item.dart` around lines 63 - 79, The code calls controller.listHorizontalActionMenuBuilder(event) twice; cache its result once in a local variable (e.g., final actions = controller.listHorizontalActionMenuBuilder(event)) and then reuse that variable for both listHorizontalActionMenu and listAction to avoid duplicated work and keep the menu representations consistent; update references to listHorizontalActionMenu and the listAction mapping (which creates ContextMenuAction(name: action.action.name)) to use the cached actions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/pages/chat/chat_event_list_item.dart`:
- Around line 63-79: The code calls
controller.listHorizontalActionMenuBuilder(event) twice; cache its result once
in a local variable (e.g., final actions =
controller.listHorizontalActionMenuBuilder(event)) and then reuse that variable
for both listHorizontalActionMenu and listAction to avoid duplicated work and
keep the menu representations consistent; update references to
listHorizontalActionMenu and the listAction mapping (which creates
ContextMenuAction(name: action.action.name)) to use the cached actions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ac61e44-215f-4a14-bbc9-816e9b086946
📒 Files selected for processing (2)
lib/pages/chat/chat_event_list_item.dartlib/utils/matrix_sdk_extensions/hive_collections_database.dart
💤 Files with no reviewable changes (1)
- lib/utils/matrix_sdk_extensions/hive_collections_database.dart
9clg6
force-pushed
the
perf/repaint-boundary-messages
branch
from
281cae3 to
f44f4b4
Compare
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/pages/chat/chat.dart`:
- Around line 3729-3735: The current code calls
PaintingBinding.instance.imageCache.clear() and clearLiveImages(), which evicts
app-wide images; remove those two calls from chat.dart (e.g., from
ChatPage/ChatState cleanup) and instead either 1) do nothing (let Flutter's LRU
handle eviction) or 2) set a lower global cap once via
PaintingBinding.instance.imageCache.maximumSize (e.g., in app init) or 3)
perform targeted eviction by calling ImageProvider.evict() for room-specific MXC
image URLs during chat cleanup (use the same place you removed the clears, e.g.,
dispose/close handler). Ensure you reference
PaintingBinding.instance.imageCache, imageCache.maximumSize, and
ImageProvider.evict when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab8bba5c-13f6-48b5-84ef-41cd3187fb1b
📒 Files selected for processing (2)
lib/pages/chat/chat.dartlib/pages/chat/chat_event_list_item.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/pages/chat/chat_event_list_item.dart
|
|
||
| // Evict decoded images from Flutter's image cache to free GPU/raster | ||
| // memory. This is important because decoded bitmaps can consume | ||
| // significantly more memory than their compressed form. | ||
| PaintingBinding.instance.imageCache.clear(); | ||
| PaintingBinding.instance.imageCache.clearLiveImages(); | ||
|
|
There was a problem hiding this comment.
Clearing the global image cache affects the entire app, not just this chat.
PaintingBinding.instance.imageCache.clear() and clearLiveImages() evict ALL cached images app-wide, including:
- Avatars in the chat list and contact list
- Images from other chat rooms the user has open
- Any UI images loaded from assets
This will cause:
- Visible flickering when navigating back to screens that had cached images
- Additional network requests to re-fetch previously cached images
- Poor UX, especially on slower connections
Flutter's image cache already uses LRU eviction to manage memory automatically. If memory is a concern for this specific chat, consider:
- Letting the default LRU behavior handle eviction naturally
- Only evicting MXC images specific to this room's messages (if a mechanism exists)
- Reducing
imageCache.maximumSizeglobally instead of clearing on every chat close
🔧 Suggested fix: Remove aggressive cache clearing
timeline?.cancelSubscriptions();
timeline = null;
-
- // Evict decoded images from Flutter's image cache to free GPU/raster
- // memory. This is important because decoded bitmaps can consume
- // significantly more memory than their compressed form.
- PaintingBinding.instance.imageCache.clear();
- PaintingBinding.instance.imageCache.clearLiveImages();
-
+ // Flutter's LRU image cache handles eviction automatically
inputFocus.dispose();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/pages/chat/chat.dart` around lines 3729 - 3735, The current code calls
PaintingBinding.instance.imageCache.clear() and clearLiveImages(), which evicts
app-wide images; remove those two calls from chat.dart (e.g., from
ChatPage/ChatState cleanup) and instead either 1) do nothing (let Flutter's LRU
handle eviction) or 2) set a lower global cap once via
PaintingBinding.instance.imageCache.maximumSize (e.g., in app init) or 3)
perform targeted eviction by calling ImageProvider.evict() for room-specific MXC
image URLs during chat cleanup (use the same place you removed the clears, e.g.,
dispose/close handler). Ensure you reference
PaintingBinding.instance.imageCache, imageCache.maximumSize, and
ImageProvider.evict when making the change.
2 participants
Summary
Wrap each
Messagewidget in aRepaintBoundaryto isolate GPU repaints per message.Problem
Each message goes through ~12 levels of widget nesting (
AutoScrollTag→Message→VisibilityDetector→Column→SwipeableMessage→ ...) without any repaint isolation. When a single message changes state (hover, reaction update, selection), the rendering engine could repaint the entire visible chat area.Solution
Add
RepaintBoundary(child: Message(...))insideAutoScrollTaginChatEventListItem. Each message now gets its own compositing layer, so state changes only trigger a repaint of that specific message.Memory cost
Each
RepaintBoundarycreates a separate layer in the render tree. However,SliverChildBuilderDelegateonly builds the ~10-15 messages currently visible on screen, so the overhead is bounded and negligible.Note on measurement
RepaintBoundaryreduces GPU overdraw, which is not directly visible in main-thread trace metrics. The improvement shows up as reduced composite/paint time in the rendering pipeline and smoother visual interactions (hover, reactions), but is difficult to quantify precisely via DevTools trace events alone. This is a standard Flutter best practice for complex list items.Files changed
lib/pages/chat/chat_event_list_item.dartMessageinRepaintBoundarySummary by CodeRabbit