fix(ui, core): decreased cognitive complexity - #2879
Open
renefloor wants to merge 2 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds ChangesMessage list layout
Paged list and grid views
Workflow quality checks
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
renefloor
changed the title
![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: 2
🧹 Nitpick comments (4)
packages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dart (3)
787-794: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the two message indices through the layout.
_buildMessageSeparatoris the last place that keeps raw slot arithmetic. Separatorindexdivides itemsindexandindex + 1, so the two message positions arelayout.messageIndexAt(index)andlayout.messageIndexAt(index + 1). Passinglayouthere removes the remaining literal offsets and keeps every index conversion in one type.The bounds stay safe either way:
betweenMessagesseparators only occur forindexin[2, messageCount].♻️ Proposed refactor
- case MessageListSeparatorSlot.betweenMessages: - return _buildMessageSeparator(context, index); + case MessageListSeparatorSlot.betweenMessages: + return _buildMessageSeparator(context, layout, index);- Widget _buildMessageSeparator(BuildContext context, int index) { + Widget _buildMessageSeparator(BuildContext context, MessageListLayout layout, int index) { // Separator `index` sits between items `index` and `index + 1`, so the two // messages it divides are offset by one and two message slots — in the // order they are rendered, which the list direction flips. + final first = layout.messageIndexAt(index); + final second = layout.messageIndexAt(index + 1); final (message, nextMessage) = switch (widget.config.reverse) { - true => (messages[index - 1], messages[index - 2]), - false => (messages[index - 2], messages[index - 1]), + true => (messages[second], messages[first]), + false => (messages[first], messages[second]), };🤖 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/message_list_view/message_list_view.dart` around lines 787 - 794, Update _buildMessageSeparator to accept or access the layout object and resolve both messages via layout.messageIndexAt(index) and layout.messageIndexAt(index + 1), removing the reverse-dependent raw index arithmetic. Preserve the existing separator behavior and safe bounds for betweenMessages separators.
890-893: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the remaining fixed offsets through the layout too.
This call site now uses
layout.itemIndexOfMessage. Three sibling sites still hard-code the same offset:_scrollToMessageusesmessages.length + 2andindex + 2, and_handleItemPositionsChangedusesconst lastItemIndex = 2. Convert them in this PR so a future slot change cannot desynchronize scrolling from rendering.#!/bin/bash # Find remaining hard-coded item-index offsets in the message list view. rg -n -C2 '\+ 2|lastItemIndex' packages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dart🤖 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/message_list_view/message_list_view.dart` around lines 890 - 893, Update _scrollToMessage and _handleItemPositionsChanged to derive all item-index offsets through MessageListLayout, replacing messages.length + 2, index + 2, and the hard-coded lastItemIndex value. Reuse the layout’s existing item-index conversion so scrolling remains synchronized with rendered slots.
751-754: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a spacing token for the edge gap height.
Both gap builders return
SizedBox(height: 8). The rest of the file resolves sizes fromcontext.streamSpacing, for example in_defaultSpacingWidgetand the list padding._buildEndEdgeGaphas noBuildContextparameter, so pass one if you adopt the token.Also applies to: 775-777
🤖 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/message_list_view/message_list_view.dart` around lines 751 - 754, Replace the hardcoded 8-pixel edge-gap height in the start and end gap builders, including _buildEndEdgeGap, with the spacing token from context.streamSpacing. Pass BuildContext into _buildEndEdgeGap if needed, while preserving the existing builder selection and Empty behavior.packages/stream_chat_flutter/lib/src/message_list_view/message_list_view_layout.dart (1)
104-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a runtime type check for strict value equality.
MessageListLayoutis not final, so the current check accepts subclasses with additional state. Useother is MessageListLayout && other.runtimeType == runtimeType, or make the classfinal.equatableis not a direct dependency ofstream_chat_flutter; do not useEquatablewithout adding the dependency and its import.🤖 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/message_list_view/message_list_view_layout.dart` around lines 104 - 108, Update MessageListLayout.operator == to require matching runtimeType in addition to the MessageListLayout type check, so subclasses with additional state are not considered equal; keep hashCode consistent with the existing messageCount-based equality and do not introduce Equatable.Source: Learnings
🤖 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 @.github/workflows/stream_flutter_workflow.yml:
- Line 79: Update the workflow step using the diff-base setting so push runs do
not construct origin/ from an empty github.base_ref. Either restrict the step to
pull_request events or select origin/master when running on push while
preserving the existing pull-request base behavior.
- Around line 77-81: Update the cognitive complexity action configuration in the
workflow so diff-base is only supplied for pull_request events, or otherwise
resolves to a valid branch/revision for push events; ensure pushes never pass
the empty origin/ value while preserving the existing threshold and
increase-failure behavior.
---
Nitpick comments:
In
`@packages/stream_chat_flutter/lib/src/message_list_view/message_list_view_layout.dart`:
- Around line 104-108: Update MessageListLayout.operator == to require matching
runtimeType in addition to the MessageListLayout type check, so subclasses with
additional state are not considered equal; keep hashCode consistent with the
existing messageCount-based equality and do not introduce Equatable.
In
`@packages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dart`:
- Around line 787-794: Update _buildMessageSeparator to accept or access the
layout object and resolve both messages via layout.messageIndexAt(index) and
layout.messageIndexAt(index + 1), removing the reverse-dependent raw index
arithmetic. Preserve the existing separator behavior and safe bounds for
betweenMessages separators.
- Around line 890-893: Update _scrollToMessage and _handleItemPositionsChanged
to derive all item-index offsets through MessageListLayout, replacing
messages.length + 2, index + 2, and the hard-coded lastItemIndex value. Reuse
the layout’s existing item-index conversion so scrolling remains synchronized
with rendered slots.
- Around line 751-754: Replace the hardcoded 8-pixel edge-gap height in the
start and end gap builders, including _buildEndEdgeGap, with the spacing token
from context.streamSpacing. Pass BuildContext into _buildEndEdgeGap if needed,
while preserving the existing builder selection and Empty behavior.
🪄 Autofix
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 Plus
Run ID: a57f6650-82e7-4c6a-95bb-7a1ea736cc1d
📒 Files selected for processing (7)
.github/workflows/stream_flutter_workflow.ymlpackages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dartpackages/stream_chat_flutter/lib/src/message_list_view/message_list_view_layout.dartpackages/stream_chat_flutter/test/src/message_list_view/message_list_view_layout_test.dartpackages/stream_chat_flutter_core/lib/src/paged_value_scroll_view.dartpackages/stream_chat_flutter_core/test/paged_value_grid_view_test.dartpackages/stream_chat_flutter_core/test/paged_value_list_view_test.dart
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2879 +/- ##
==========================================
+ Coverage 73.47% 73.68% +0.21%
==========================================
Files 431 432 +1
Lines 27827 27871 +44
==========================================
+ Hits 20445 20536 +91
+ Misses 7382 7335 -47 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
📊 Cognitive Complexity AnalysisNet Delta: -111 | Added: 23 | Increased: 0 | Improved: 4 | Violations: 0
|
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
1 participant
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.
Submit a pull request
Linear: FLU-480
CLA
Description of the pull request
I made this PR by running the cognitive_complexity plugin from Kevin Moore.
See also: https://x.com/CFDevelop/status/2084725359942250776
This PR also adds a github action step in the analysis workflow to make sure new features don't have a big complexity.
The PR tried to address the complexity of the most important and most complex widget, the MessageListView.
This was the result:
Actionable hotspots (hand-written)
Scanned with
dart run cognitive_complexity@0.2.3 --threshold 15over all packagelib/roots. Production target: score <= 15._StreamMessageListViewState._buildListViewpackages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dart:591StreamMessageActionsBuilder.buildActionspackages/stream_chat_flutter/lib/src/message_action/message_actions_builder.dart:54_PagedValueGridViewState.buildpackages/stream_chat_flutter_core/lib/src/paged_value_scroll_view.dart:651LoggingInterceptor._printPrettyMappackages/stream_chat/lib/src/core/http/interceptor/logging_interceptor.dart:245DefaultStreamMessageItem.buildpackages/stream_chat_flutter/lib/src/message_widget/stream_message_item.dart:453streamChatComponentBuilderspackages/stream_chat_flutter/lib/src/components/stream_chat_component_builders.dart:4_PagedValueListViewState.buildpackages/stream_chat_flutter_core/lib/src/paged_value_scroll_view.dart:290tabbedAttachmentPickerBuilderpackages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker.dart:369StreamPollController.validateGranularlypackages/stream_chat_flutter_core/lib/src/stream_poll_controller.dart:114FloatingDateDivider._floatingDividerOpacitypackages/stream_chat_flutter/lib/src/message_list_view/floating_date_divider.dart:98_LazyLoadScrollViewState._onNotificationpackages/stream_chat_flutter_core/lib/src/lazy_load_scroll_view.dart:61Channel._uploadAttachmentspackages/stream_chat/lib/src/client/channel.dart:611Channel.querypackages/stream_chat/lib/src/client/channel.dart:2101Tail (15–27, 20 more declarations) is mostly Flutter
buildmethods instream_chat_flutter. Lower priority, but 4 breach the 5-level nesting ceiling.Strategy key
if/elseorswitchstatements with a Dart 3 switch expressionNotes
index-slot arithmetic (
itemCount - 1/-2/-3,i == 0/1,i - 2) duplicated acrossthree nested closures —
itemKeyBuilder,separatorBuilder,itemBuilder— eachre-deriving the same layout, guided by a 16-line ASCII comment block.
if (x != null)collection-ifs in onelist literal, zero nesting. The per-parameter static type is required to construct
StreamComponentBuilderExtension, so a.nonNullscollapse isn't available.Recommend accepting as-is.
if (capability) add(action)blocksat depth 1 — breadth, not nesting. Satisfying the metric here wouldn't make it easier
to read.
Excluded from remediation
.g.dartstream_chat_persistence, 2 instream_chatanalysis_options.yamlscrollable_positioned_list/Summary by CodeRabbit
Improvements
Bug Fixes