fix: implement sorting functionality for resource tree view#224
Open
jmischler72 wants to merge 6 commits intodarksworm:mainfrom
Open
fix: implement sorting functionality for resource tree view#224jmischler72 wants to merge 6 commits intodarksworm:mainfrom
jmischler72 wants to merge 6 commits intodarksworm:mainfrom
Conversation
WalkthroughIntroduces a generic Sortable-based sorting utility and a model SortKey, adds TreeView sorting support (SetSort, SortKey on nodes, re-sorting during tree upsert and status updates), propagates UI sort state from app input/model, and adds unit and e2e tests plus a .gitignore rule for e2e snapshots. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Input Handler
participant Model
participant TreeView
participant Renderer
User->>UI: Issue sort command (e.g., health asc)
UI->>Model: Update m.state.UI.Sort
UI->>TreeView: Call SetSort(sortConfig)
alt Tree view active
TreeView->>TreeView: Set sortConfig and call sortNodeChildren on roots & descendants
TreeView->>TreeView: Rebuild DFS/display order
TreeView->>Renderer: Trigger UI refresh
end
Renderer->>User: Rendered sorted tree
Note over Model,TreeView: On ResourceTreeLoadedMsg, Model applies m.state.UI.Sort to TreeView
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/tui/treeview/treeview.go (1)
318-328: Consider re-sorting when health status changes too.
SetResourceStatusesonly triggers re-sort when sorting by sync status. However, if health status can also be updated through this path (or a similar path in the future), sorting by health would not reflect those updates. Currently this appears correct sinceSetResourceStatusesonly updates sync status, but this could be a source of bugs if health updates are added later.📝 Consider future-proofing
If health updates might come through a similar path in the future, consider:
- // If sorting by sync status, re-sort to reflect updated values - if v.sortConfig != nil && v.sortConfig.Field == model.SortFieldSync { + // Re-sort if sorting by a status field that may have changed + if v.sortConfig != nil && (v.sortConfig.Field == model.SortFieldSync || v.sortConfig.Field == model.SortFieldHealth) {Or add a comment explaining why only sync is handled here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tui/treeview/treeview.go` around lines 318 - 328, The code in SetResourceStatuses only re-sorts when v.sortConfig.Field == model.SortFieldSync, which misses updates if health status becomes part of this path; update the condition in SetResourceStatuses to also trigger when v.sortConfig.Field == model.SortFieldHealth (or the equivalent enum) so that v.sortNodeChildren(roots), iterating nodesByUID children and v.rebuildOrder() run for health-based sorting too, or alternatively add a clear comment on SetResourceStatuses explaining why only sync is handled to future-proof the code; locate this logic around the SetResourceStatuses method and the symbols v.sortConfig.Field, model.SortFieldSync, sortNodeChildren, nodesByUID, roots and rebuildOrder.pkg/sort/sort.go (1)
114-133: Consider handlingSortFieldNameexplicitly for clarity.When
config.FieldisSortFieldName, the switch statement doesn't match any case, leavingcmp = 0. This causes the function to fall through to the tiebreak logic (kind, then name), which is correct behavior. However, explicit handling would improve readability and make the intent clearer.♻️ Suggested clarification
less := func(a, b T) bool { var cmp int switch config.Field { case model.SortFieldHealth: cmp = compareHealthStatus(a.NodeHealth(), b.NodeHealth()) case model.SortFieldSync: cmp = compareSyncStatus(a.NodeSync(), b.NodeSync()) + case model.SortFieldName: + // Fall through to tiebreak by (kind, name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sort/sort.go` around lines 114 - 133, The comparator "less" currently omits an explicit case for model.SortFieldName causing reliance on tiebreak logic; add a case for model.SortFieldName in the switch that sets cmp by comparing a.NodeName() and b.NodeName() (e.g., lexicographic comparison) so the name-based sort is explicit, then keep the existing Direction check (config.Direction == model.SortDesc) to invert ordering when needed and preserve the existing kind/name tiebreak fallback when cmp == 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/sort/sort.go`:
- Around line 128-132: The tiebreak currently always compares a.NodeKind() and
a.NodeName() in ascending order; update the comparator that handles ties (where
cmp == 0) to honor config.Direction by inverting the kind/name comparison when
config.Direction indicates descending (e.g., apply the same direction multiplier
or flip the comparison operands), referencing the existing config.Direction,
NodeKind(), and NodeName() symbols so ties follow the overall sort direction;
alternatively, if you choose stability over direction, add a clear
comment/docstring next to the tie-break logic stating that kind/name tiebreaks
are intentionally always ascending.
---
Nitpick comments:
In `@pkg/sort/sort.go`:
- Around line 114-133: The comparator "less" currently omits an explicit case
for model.SortFieldName causing reliance on tiebreak logic; add a case for
model.SortFieldName in the switch that sets cmp by comparing a.NodeName() and
b.NodeName() (e.g., lexicographic comparison) so the name-based sort is
explicit, then keep the existing Direction check (config.Direction ==
model.SortDesc) to invert ordering when needed and preserve the existing
kind/name tiebreak fallback when cmp == 0.
In `@pkg/tui/treeview/treeview.go`:
- Around line 318-328: The code in SetResourceStatuses only re-sorts when
v.sortConfig.Field == model.SortFieldSync, which misses updates if health status
becomes part of this path; update the condition in SetResourceStatuses to also
trigger when v.sortConfig.Field == model.SortFieldHealth (or the equivalent
enum) so that v.sortNodeChildren(roots), iterating nodesByUID children and
v.rebuildOrder() run for health-based sorting too, or alternatively add a clear
comment on SetResourceStatuses explaining why only sync is handled to
future-proof the code; locate this logic around the SetResourceStatuses method
and the symbols v.sortConfig.Field, model.SortFieldSync, sortNodeChildren,
nodesByUID, roots and rebuildOrder.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignorecmd/app/input_components.gocmd/app/model.goe2e/sort_test.gopkg/sort/sort.gopkg/tui/treeview/treeview.gopkg/tui/treeview/treeview_test.go
Greptile SummaryExtends sorting functionality to resource tree view by refactoring sort logic to use a generic Key changes:
Confidence Score: 5/5
Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class Sortable {
<<interface>>
+SortKey() SortKey
}
class SortKey {
+Health string
+Sync string
+Kind string
+Name string
}
class App {
+Name string
+Health string
+Sync string
+SortKey() SortKey
}
class treeNode {
+name string
+kind string
+health string
+status string
+SortKey() SortKey
}
class TreeView {
-sortConfig *SortConfig
-roots []*treeNode
-nodesByUID map
+SetSort(config SortConfig)
+sortNodeChildren(list []*treeNode)
}
class Sort~T~ {
<<generic function>>
+Sort(items []T, config SortConfig)
}
Sortable <|.. App : implements
Sortable <|.. treeNode : implements
App ..> SortKey : returns
treeNode ..> SortKey : returns
TreeView --> treeNode : contains
TreeView --> SortConfig : uses
Sort~T~ ..> Sortable : constraint
Last reviewed commit: 46e8749 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/tui/treeview/treeview.go (1)
102-110: Misleading comment about default sort order.The comment states "default (kind, name) ascending order" but the code sets
Field: model.SortFieldName. Based on theSortTreeNodesimplementation (frompkg/sort/sort.go), the primary sort is by name, with kind/name as tiebreakers. The comment should be updated to accurately reflect this behavior.📝 Suggested comment fix
-// sortNodeChildren sorts a sibling list using the current sort config via sort.SortTreeNodes. -// A nil sortConfig resolves to the default (kind, name) ascending order. +// sortNodeChildren sorts a sibling list using the current sort config via pkgsort.SortTreeNodes. +// A nil sortConfig resolves to the default (name ascending) order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tui/treeview/treeview.go` around lines 102 - 110, Update the misleading comment above sortNodeChildren: it currently claims the default is "(kind, name) ascending" but the code (and pkgsort.SortTreeNodes behavior) uses name as the primary key with kind/name as tiebreakers; change the comment to state that a nil sortConfig resolves to the default of sorting by name ascending, using kind (and then name as necessary) as tiebreakers (or otherwise reflect the exact tiebreak order), and keep references to TreeView.sortNodeChildren, model.SortConfig/model.SortFieldName, and pkgsort.SortTreeNodes so reviewers can verify the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/tui/treeview/treeview.go`:
- Around line 318-328: The re-sort conditional in SetResourceStatuses
incorrectly includes model.SortFieldHealth even though this method only updates
node.status (see SetResourceStatuses and the node.status assignment) and not
health; remove model.SortFieldHealth from the condition so it only checks
v.sortConfig.Field == model.SortFieldSync before calling
v.sortNodeChildren(v.roots), iterating v.nodesByUID to
v.sortNodeChildren(node.children), and then v.rebuildOrder; this avoids
unnecessary re-sorting when the sort key is health which isn’t changed here.
---
Nitpick comments:
In `@pkg/tui/treeview/treeview.go`:
- Around line 102-110: Update the misleading comment above sortNodeChildren: it
currently claims the default is "(kind, name) ascending" but the code (and
pkgsort.SortTreeNodes behavior) uses name as the primary key with kind/name as
tiebreakers; change the comment to state that a nil sortConfig resolves to the
default of sorting by name ascending, using kind (and then name as necessary) as
tiebreakers (or otherwise reflect the exact tiebreak order), and keep references
to TreeView.sortNodeChildren, model.SortConfig/model.SortFieldName, and
pkgsort.SortTreeNodes so reviewers can verify the behavior.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/tui/treeview/treeview.go (1)
318-327:⚠️ Potential issue | 🟡 MinorRe-sort trigger should not include health in
SetResourceStatuses.
SetResourceStatusesupdates sync status (node.status) only. IncludingSortFieldHealthat Line 319 causes unnecessary full-tree re-sorts when health values did not change.🛠️ Suggested fix
- // If sorting by sync or health status, re-sort to reflect updated values - if v.sortConfig != nil && (v.sortConfig.Field == model.SortFieldSync || v.sortConfig.Field == model.SortFieldHealth) { + // If sorting by sync status, re-sort to reflect updated values + if v.sortConfig != nil && v.sortConfig.Field == model.SortFieldSync { v.sortNodeChildren(v.roots) for _, node := range v.nodesByUID { if len(node.children) > 0 { v.sortNodeChildren(node.children) } } v.rebuildOrder() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tui/treeview/treeview.go` around lines 318 - 327, The conditional in SetResourceStatuses currently triggers a full re-sort when v.sortConfig.Field is model.SortFieldHealth even though SetResourceStatuses only updates sync status; remove model.SortFieldHealth from the check so the branch only runs for model.SortFieldSync (i.e., change the conditional to check only for SortFieldSync), leaving the calls to v.sortNodeChildren(...) and v.rebuildOrder() intact so sorts still occur when sync status changes.
🧹 Nitpick comments (1)
pkg/sort/sort.go (1)
31-33: Comparator docs should match actual precedence chain.Current comments imply direct kind/name tiebreaking, but for health/sync ties you first compare by name (Lines 47-50). Consider updating comments/docstrings to reflect the real order to avoid confusion.
Also applies to: 47-50, 59-63, 105-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sort/sort.go` around lines 31 - 33, Update the misleading comment on comparatorGeneric to reflect the actual precedence chain: it applies semantic health/sync ordering and, on ties, tiebreaks by name first and then by kind (not kind then name); find and correct the same phrasing in the nearby comparator/doc comments in the same file (the other comparator functions and comment blocks that reference "tiebreaks by kind then name") so all docs match the implemented comparison order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/tui/treeview/treeview.go`:
- Around line 318-327: The conditional in SetResourceStatuses currently triggers
a full re-sort when v.sortConfig.Field is model.SortFieldHealth even though
SetResourceStatuses only updates sync status; remove model.SortFieldHealth from
the check so the branch only runs for model.SortFieldSync (i.e., change the
conditional to check only for SortFieldSync), leaving the calls to
v.sortNodeChildren(...) and v.rebuildOrder() intact so sorts still occur when
sync status changes.
---
Nitpick comments:
In `@pkg/sort/sort.go`:
- Around line 31-33: Update the misleading comment on comparatorGeneric to
reflect the actual precedence chain: it applies semantic health/sync ordering
and, on ties, tiebreaks by name first and then by kind (not kind then name);
find and correct the same phrasing in the nearby comparator/doc comments in the
same file (the other comparator functions and comment blocks that reference
"tiebreaks by kind then name") so all docs match the implemented comparison
order.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/app/view.gopkg/model/sort.gopkg/model/types.gopkg/sort/sort.gopkg/tui/treeview/treeview.go
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.
This pr Closes #220
It fixes by doing a sort on the resources (main tree nodes then sort childrens for each nodes)
In draft, because i am trying to generalize with the existing sort logic
Summary by CodeRabbit
New Features
Tests
Chores