-
-
Notifications
You must be signed in to change notification settings - Fork 447
fix: cant sort servers order #930
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds server order update logic to ServersNotifier and refactors the server sequence settings page to manage local order state and correctly handle drag-and-drop reordering. Also updates Riverpod-generated hash strings for several providers with no behavioral changes. Changes
Sequence Diagram(s)sequenceDiagram
actor U as User
participant P as Server Seq Page
participant R as ReorderableListView
participant N as ServersNotifier
participant S as Storage
participant SY as Sync/Scheduler
U->>R: Drag item to new position
R-->>P: onReorder(oldIndex, newIndex)
rect rgba(200,230,255,0.3)
note right of P: Compute adjusted targetIndex and newOrder
P->>P: setState(_order = newOrder)
P->>N: updateServerOrder(newOrder)
end
N->>N: _isSameOrder? (fast path)
alt Order changed
N->>S: Persist serverOrder
N-->>SY: Trigger synchronization
else No change
N-->>P: No-op
end
note over N,P: Page also listens to serversProvider changes<br/>and syncs local _order when external updates occur.
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/data/provider/server/all.dart (1)
271-284
: Consider using collection equality from fl_lib.The manual list equality implementation is correct with the
identical()
fast-path optimization. However, sincefl_lib
is already imported (line 3), you could use the.equals()
extension method instead:bool _isSameOrder(List<String> a, List<String> b) => a.equals(b);The current implementation is fine and avoids any potential overhead, so this is purely a stylistic suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/data/provider/pve.g.dart
(1 hunks)lib/data/provider/server/all.dart
(1 hunks)lib/data/provider/server/all.g.dart
(1 hunks)lib/data/provider/server/single.g.dart
(1 hunks)lib/data/provider/systemd.g.dart
(1 hunks)lib/view/page/setting/seq/srv_seq.dart
(3 hunks)
🔇 Additional comments (10)
lib/data/provider/server/all.g.dart (1)
44-44
: Generated file - no review needed.This is a Riverpod-generated hash update corresponding to changes in the source file
lib/data/provider/server/all.dart
. Generated files are automatically updated by the build runner.lib/data/provider/pve.g.dart (1)
61-61
: Generated file - no review needed.This is a Riverpod-generated hash update. Generated files are automatically updated by the build runner and don't require manual review.
lib/data/provider/systemd.g.dart (1)
61-61
: Generated file - no review needed.This is a Riverpod-generated hash update. No manual review required.
lib/data/provider/server/single.g.dart (1)
61-61
: Generated file - no review needed.This is a Riverpod-generated hash update. Generated files don't require manual review.
lib/data/provider/server/all.dart (1)
242-269
: LGTM - well-implemented server order update logic.The method correctly:
- Deduplicates the input order and filters out invalid server IDs (lines 246-254)
- Ensures all existing servers are included by appending missing IDs (lines 256-260)
- Early exits if the order hasn't changed (lines 262-264)
- Updates state, persists to storage, and triggers sync (lines 266-268)
This addresses the core issue in #927 by providing a proper persistence mechanism for reordered servers.
lib/view/page/setting/seq/srv_seq.dart (5)
20-26
: LGTM - proper local state initialization.Initializing local
_order
from the provider ininitState
is the correct approach. This provides immediate UI updates during drag operations while maintaining sync with the provider state.
30-37
: LGTM - correct Riverpod listener pattern.The listener properly syncs local state with provider changes while preventing infinite loops via the
listEquals
check. Placingref.listen
in the build method is the correct Riverpod pattern - the framework handles subscription lifecycle automatically.
70-86
: LGTM - correct reorder logic that fixes the bug.The reorder implementation correctly:
- Calculates
targetIndex
accounting for the index shift when dragging downward (lines 72-73)- Early exits if no actual reorder occurred (lines 75-77)
- Creates the new order by removing and reinserting the item (lines 79-81)
- Updates local state immediately for responsive UI (lines 83-85)
- Persists changes via the provider (line 86)
This fixes issue #927 by maintaining local state that updates immediately, preventing the item from reverting to its original position.
90-94
: LGTM - proper data fetching and null handling.The item builder now correctly fetches server data (
Spi
) by looking up the ID from the order list in the server state. The null check is good defensive programming that handles edge cases where a server might be briefly missing during deletion or sync operations.
111-114
: Good defensive null handling.Returning an empty
SizedBox
for nullSpi
gracefully handles edge cases where a server ID exists in the order but not in the server state (e.g., during brief sync windows when a server is deleted). This prevents crashes and UI glitches.
Fixes: #927
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style