feat: add a scroll bar to the tracks list #283
Merged
Conversation
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
Reviewer's GuideThis PR refactors all track list screens to remove the previous infinite_scroll_pagination dependency and replace it with a manual lazy-loading mechanism driven by a totalCount/getItem interface wrapped in Scrollbar widgets, and adds a new backend signal and Dart utility to fetch the total number of media files for correct scroll positioning. Sequence diagram for manual lazy loading and scroll bar initializationsequenceDiagram
actor User
participant UI
participant Backend
participant Database
User->>UI: Open track list page
UI->>Backend: Request total track count
Backend->>Database: Query media files count
Database-->>Backend: Return count
Backend-->>UI: Return total count
UI->>UI: Display scroll bar with correct total count
UI->>Backend: Request first page of tracks
Backend->>Database: Query first page of tracks
Database-->>Backend: Return tracks
Backend-->>UI: Return tracks
UI->>UI: Display first page of tracks
User->>UI: Scrolls to end of list
UI->>Backend: Request next page of tracks
Backend->>Database: Query next page of tracks
Database-->>Backend: Return tracks
Backend-->>UI: Return tracks
UI->>UI: Display next page of tracks
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Extract the custom lazy‐loading and scroll logic into a shared controller or widget to avoid duplicating pagination code across different screen layouts.
- Avoid using an arbitrary fallback totalCount (e.g. 10000) when the count fetch fails—consider surfacing an error state or using a clearer default strategy.
- Review the use of SchedulerBinding.addPostFrameCallback for state updates—batch your setState calls where possible to simplify the update flow and reduce scheduling overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the custom lazy‐loading and scroll logic into a shared controller or widget to avoid duplicating pagination code across different screen layouts.
- Avoid using an arbitrary fallback totalCount (e.g. 10000) when the count fetch fails—consider surfacing an error state or using a clearer default strategy.
- Review the use of SchedulerBinding.addPostFrameCallback for state updates—batch your setState calls where possible to simplify the update flow and reduce scheduling overhead.
## Individual Comments
### Comment 1
<location> `lib/widgets/track_list/small_screen_track_list.dart:101-103` </location>
<code_context>
),
- ),
+ // Add extra bottom spacing for the last item
+ if (isLastItem)
+ SizedBox(height: MediaQuery.of(context).size.width / 3 + 20),
+ ],
);
</code_context>
<issue_to_address>
**suggestion:** Adding extra bottom spacing based on screen width may not scale well on all devices.
The current calculation may lead to inconsistent padding across devices. Consider using a fixed value, basing it on screen height, or adjusting it according to content layout for better responsiveness.
```suggestion
// Add extra bottom spacing for the last item
if (isLastItem)
SizedBox(height: MediaQuery.of(context).size.height * 0.05),
```
</issue_to_address>
### Comment 2
<location> `lib/widgets/track_list/large_screen_track_list.dart:85-86` </location>
<code_context>
+ itemBuilder: (context, index) {
+ final item = getItem(index);
+
+ if (item == null) {
+ return const SizedBox(
+ width: 64,
</code_context>
<issue_to_address>
**suggestion:** Displaying a ProgressRing for null items may result in multiple spinners in the grid.
Consider using a single loading indicator for the grid or a skeleton loader for individual items to avoid multiple spinners appearing simultaneously.
</issue_to_address>
### Comment 3
<location> `lib/widgets/track_list/band_screen_track_list.dart:92-93` </location>
<code_context>
+ itemBuilder: (context, index) {
+ final item = getItem(index);
+
+ if (item == null) {
+ return const SizedBox(
+ width: 64,
+ height: 64,
</code_context>
<issue_to_address>
**suggestion:** Returning a fixed-size loading indicator may cause layout jumps if items load at different rates.
Consider using a placeholder that matches the final item's dimensions to prevent layout shifts when content loads.
Suggested implementation:
```
if (item == null) {
// Use the same dimensions as BandViewTrackItem to prevent layout shifts.
return const SizedBox(
width: BandViewTrackItem.width,
height: BandViewTrackItem.height,
child: Center(child: ProgressRing()),
);
}
```
- If `BandViewTrackItem.width` and `BandViewTrackItem.height` are not defined as static constants, you should define them in the `BandViewTrackItem` class and use those values here.
- If the dimensions are dynamic or depend on the context, you may need to refactor both the placeholder and the item widget to use a shared method or constant for sizing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+101
to
+103
| // Add extra bottom spacing for the last item | ||
| if (isLastItem) | ||
| SizedBox(height: MediaQuery.of(context).size.width / 3 + 20), |
There was a problem hiding this comment.
suggestion: Adding extra bottom spacing based on screen width may not scale well on all devices.
The current calculation may lead to inconsistent padding across devices. Consider using a fixed value, basing it on screen height, or adjusting it according to content layout for better responsiveness.
Suggested change
| // Add extra bottom spacing for the last item | |
| if (isLastItem) | |
| SizedBox(height: MediaQuery.of(context).size.width / 3 + 20), | |
| // Add extra bottom spacing for the last item | |
| if (isLastItem) | |
| SizedBox(height: MediaQuery.of(context).size.height * 0.05), |
Comment on lines
+85
to
+86
| if (item == null) { | ||
| return const Center(child: ProgressRing()); |
There was a problem hiding this comment.
suggestion: Displaying a ProgressRing for null items may result in multiple spinners in the grid.
Consider using a single loading indicator for the grid or a skeleton loader for individual items to avoid multiple spinners appearing simultaneously.
Comment on lines
+92
to
+93
| if (item == null) { | ||
| return const SizedBox( |
There was a problem hiding this comment.
suggestion: Returning a fixed-size loading indicator may cause layout jumps if items load at different rates.
Consider using a placeholder that matches the final item's dimensions to prevent layout shifts when content loads.
Suggested implementation:
if (item == null) {
// Use the same dimensions as BandViewTrackItem to prevent layout shifts.
return const SizedBox(
width: BandViewTrackItem.width,
height: BandViewTrackItem.height,
child: Center(child: ProgressRing()),
);
}
- If
BandViewTrackItem.widthandBandViewTrackItem.heightare not defined as static constants, you should define them in theBandViewTrackItemclass and use those values here. - If the dimensions are dynamic or depend on the context, you may need to refactor both the placeholder and the item widget to use a shared method or constant for sizing.
Owner
|
Hi, I noticed that you are still editing the pull request, let me know when you are ready for the review, thanks! |
Contributor
Author
Let me attempt to fix the analyze problems first, thanks! When I complete I will reply you again. @Losses |
Contributor
Author
|
Now I've completed my fix. Seems that rust analyze fails mainly due to other files, so maybe it is required to ignore its result currently. You can review the code now. Thanks! @Losses |
Owner
I'll merge it first and tweak it later, thank you for the contribution! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This pull request added a scroll bar to the track list page, which is convenient in many situation. For example, if one just want to manually randomly select a track to listen.
Summary by Sourcery
Add scrollbars to all track list screens with an accurate total count by querying a new media files count endpoint and refactor away from infinite_scroll_pagination in favor of manual lazy-loading builders
New Features:
Enhancements: