fix: several bugs around collection displaying & editing #285
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 resolves crashes and incomplete track listings in the Mix Studio modal and track list pages by overhauling pagination and lazy loading, corrects collection type dispatch for summary requests, and adds extensive logging and error handling across the client and server stacks for easier debugging. Sequence diagram for lazy loading and pagination in Mix Studio modalsequenceDiagram
actor User
participant "MixStudioDialog"
participant "MixEditorController"
participant "queryMixTracks()"
participant "TrackListGrid"
User->>MixStudioDialog: Open Mix Studio modal
MixStudioDialog->>MixEditorController: loadMix(mixId)
MixEditorController->>MixStudioDialog: setData(queryData)
MixStudioDialog->>queryMixTracks(): Query tracks (cursor=0, pageSize)
queryMixTracks()-->>MixStudioDialog: Return first page of tracks
MixStudioDialog->>TrackListGrid: Display tracks
User->>TrackListGrid: Scrolls down
TrackListGrid->>MixStudioDialog: onScroll event
MixStudioDialog->>queryMixTracks(): Query tracks (next cursor, pageSize)
queryMixTracks()-->>MixStudioDialog: Return next page of tracks
MixStudioDialog->>TrackListGrid: Append tracks
Class diagram for updated MixStudioDialogImplementationState and related pagination logicclassDiagram
class MixStudioDialogImplementationState {
-MixEditorController _controller
-StartScreenLayoutManager _layoutManager
-ScrollController _scrollController
-bool isLoading
-String _query
-int _cursor
-bool _isLoadingMore
-bool _hasMore
-List<InternalMediaFile> _allTracks
-Timer? _debounceTimer
-bool _isInitialLoad
+void initState()
+void dispose()
+Future<void> loadMix(int mixId)
+void _debouncedResetAndLoad()
+void _onScroll()
+Future<void> _resetAndLoadTracks()
+Future<void> _loadMoreTracks()
}
class MixEditorController {
+void setData(queryData)
+void addListener(listener)
+queryData getData()
}
class InternalMediaFile {
+int id
+other attributes
}
MixStudioDialogImplementationState "1" o-- "1" MixEditorController
MixStudioDialogImplementationState "1" o-- "1" StartScreenLayoutManager
MixStudioDialogImplementationState "1" o-- "1" ScrollController
MixStudioDialogImplementationState "*" o-- "*" InternalMediaFile
Class diagram for SearchCollectionSummaryRequest changesclassDiagram
class SearchCollectionSummaryRequest {
+CollectionType collectionType
+int n
+Future<Response> sendSignalToRust()
}
class SearchCollectionSummaryResponse {
+Stream rustSignalStream
+Response message
}
SearchCollectionSummaryRequest --> SearchCollectionSummaryResponse
Class diagram for updated VirtualFS error handling and loggingclassDiagram
class VirtualFS {
+Result path_to_query(Path)
+Result resolve_path_with_ids(Path)
+Result validate_path(Path)
+void cache_entries(Path, entries, CollectionType)
+Map cache
}
class Path {
+int components().count()
+Path parent()
+String file_name()
}
VirtualFS o-- Path
File-Level Changes
Possibly linked issues
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/screens/query_tracks/query_tracks_list.dart:50-56` </location>
<code_context>
+ // Pre-load first page before marking as initialized
+ await _loadPageForInitialization(0);
</code_context>
<issue_to_address>
**suggestion (performance):** Blocking initialization on data load may impact perceived performance.
Initializing the UI before loading data and displaying a loading indicator can enhance responsiveness, especially on slow networks.
```suggestion
Future<void> _initializeData() async {
// Mark as initialized before loading data
setState(() {
_isInitialized = true;
});
// Start loading first page, but don't block initialization
_loadPageForInitialization(0);
}
```
</issue_to_address>
### Comment 2
<location> `native/hub/src/server/client/fs.rs:189-193` </location>
<code_context>
let collection_type = path_to_collection_type(&parent_path)
- .ok_or_else(|| anyhow!("Invalid collection type"))?;
+ .ok_or_else(|| {
+ log::error!("resolve_path_with_ids: Failed to determine collection type for parent_path: {:?}", parent_path);
+ anyhow!("Invalid collection type for path: {:?}", parent_path)
+ })?;
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Logging errors with potentially large or sensitive paths may have privacy or performance implications.
Sanitize or truncate paths before logging to avoid exposing sensitive data and generating excessively large logs.
```suggestion
fn sanitize_path_for_log(path: &std::path::Path) -> String {
// Truncate to 100 chars and remove sensitive components
let path_str = path.to_string_lossy();
let sanitized = if path_str.len() > 100 {
format!("{}...[truncated]", &path_str[..100])
} else {
path_str.to_string()
};
sanitized.replace("\\", "/") // Normalize separators
}
let collection_type = path_to_collection_type(&parent_path)
.ok_or_else(|| {
let sanitized_path = sanitize_path_for_log(&parent_path);
log::error!(
"resolve_path_with_ids: Failed to determine collection type for parent_path: {}",
sanitized_path
);
anyhow!("Invalid collection type for path: {}", sanitized_path)
})?;
```
</issue_to_address>
### Comment 3
<location> `database/src/actions/mixes.rs:727` </location>
<code_context>
cursor: usize,
page_size: usize,
) -> Result<Vec<media_files::Model>> {
+ log::info!("query_mix_media_files: cursor={}, page_size={}, queries={:?}", cursor, page_size, queries);
+
let mut all: bool = false;
</code_context>
<issue_to_address>
**suggestion:** Info-level logging for every query may be excessive in production.
Consider lowering the log level or adding log sampling to prevent excessive logging if this function is called often.
Suggested implementation:
```rust
log::debug!("query_mix_media_files: cursor={}, page_size={}, queries={:?}", cursor, page_size, queries);
```
```rust
log::debug!("query_mix_media_files: returning {} media files", media_files.len());
```
If you want to implement log sampling (e.g., only log every Nth call), you would need to introduce a counter or use a sampling library. If you want this, let me know and I can provide a code example.
</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
50
to
+56
| Future<void> _initializeData() async { | ||
| // Pre-load first page before marking as initialized | ||
| await _loadPageForInitialization(0); | ||
| setState(() { | ||
| _isInitialized = true; | ||
| }); | ||
| // Pre-load first page | ||
| _loadPage(0); | ||
| } |
There was a problem hiding this comment.
suggestion (performance): Blocking initialization on data load may impact perceived performance.
Initializing the UI before loading data and displaying a loading indicator can enhance responsiveness, especially on slow networks.
Suggested change
| Future<void> _initializeData() async { | |
| // Pre-load first page before marking as initialized | |
| await _loadPageForInitialization(0); | |
| setState(() { | |
| _isInitialized = true; | |
| }); | |
| // Pre-load first page | |
| _loadPage(0); | |
| } | |
| Future<void> _initializeData() async { | |
| // Mark as initialized before loading data | |
| setState(() { | |
| _isInitialized = true; | |
| }); | |
| // Start loading first page, but don't block initialization | |
| _loadPageForInitialization(0); | |
| } |
database/src/actions/mixes.rs
Outdated
| cursor: usize, | ||
| page_size: usize, | ||
| ) -> Result<Vec<media_files::Model>> { | ||
| log::info!("query_mix_media_files: cursor={}, page_size={}, queries={:?}", cursor, page_size, queries); |
There was a problem hiding this comment.
suggestion: Info-level logging for every query may be excessive in production.
Consider lowering the log level or adding log sampling to prevent excessive logging if this function is called often.
Suggested implementation:
log::debug!("query_mix_media_files: cursor={}, page_size={}, queries={:?}", cursor, page_size, queries); log::debug!("query_mix_media_files: returning {} media files", media_files.len());If you want to implement log sampling (e.g., only log every Nth call), you would need to introduce a counter or use a sampling library. If you want this, let me know and I can provide a code example.
Owner
|
Hey! Thank you for the fix! I think I can merge it after some tweaks. I agree with Sourcey AI's suggestion about logs. Too much log will affect the normal debugging process. If you are sure that the bug has been fixed, please reduce the log level and avoid accumulating a large amount of logs in the Terminal. |
Contributor
Author
|
Ok, I will delete the logs later |
Contributor
Author
|
Now I've deleted many of the unused log function calls, and have tested that the program works still well now. |
Owner
|
lgtm, thx! |
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.
In my environment (Arch Linux), when I attempt 2.0.0alpha9 I noticed several bugs including:
In this PR, I've done the following:
SearchCollectionSummaryRequestto deliver the correct collection type, and process them correctly.Summary by Sourcery
Fix several collection display and editing issues by correcting collection type handling, implementing proper pagination for mix and track views, and adding extensive logging to aid debugging.
Bug Fixes:
Enhancements: