Skip to content

fix: Feat: Sorting when using a filter#230

Merged
sammcj merged 2 commits intosammcj:mainfrom
majiayu000:fix-166-feat-sorting-when-using-a-filter
Dec 30, 2025
Merged

fix: Feat: Sorting when using a filter#230
sammcj merged 2 commits intosammcj:mainfrom
majiayu000:fix-166-feat-sorting-when-using-a-filter

Conversation

@majiayu000
Copy link
Contributor

Summary

This PR fixes #166

Changes

  • app_model.go

When a filter is active, sorting now preserves the filtered view
instead of clearing it. This fixes issue sammcj#166 where sorting would
break when using a filter.

Added refreshListWithSort() function that:
- Sorts the master models list
- If a filter is active, also sorts the visible filtered items
- Preserves cursor position after sorting

Also fixed a pre-existing build error where promptForNewName() return
value was not properly handled.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Signed-off-by: majiayu000 <1835304752@qq.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes issue #166 by implementing proper sorting behavior when a filter is active. Previously, sorting operations would lose the active filter state by replacing all list items with the sorted master list. The fix introduces a new refreshListWithSort method that preserves filter state while sorting both the master list and visible filtered items.

Key changes:

  • Added refreshListWithSort method to handle sorting while preserving active filters
  • Refactored all sort handlers to use the new method instead of calling sort.Slice + refreshList separately
  • Added cancellation handling for model creation from Modelfile (which was already present in copy and rename operations)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Optimizations:
- Replace O(n²) nested loop with O(n) HashMap lookup for better performance
- Add early return for empty filtered lists
- Remove unnecessary nil assignment to prevent UI flicker
- Add proper variable initialization with foundI/foundJ checks

Bug fixes:
- Add empty name validation after cancellation in handleEditorFinishedMsg
- Improve cursor position boundary check (only select if index >= 0)

These changes address code review feedback from GitHub Copilot while
maintaining the core functionality of preserving filter state during sorting.
@sammcj
Copy link
Owner

sammcj commented Dec 30, 2025

fyi some copilot comments @majiayu000, they may not all be valid but could you check?

@majiayu000
Copy link
Contributor Author

Hi @sammcj, I've reviewed the Copilot comments and addressed them in the latest commit:

  1. O(n²) performance issue ✅ - Replaced nested loops with HashMap lookup for O(1) complexity
  2. Empty name validation ✅ - Added strings.TrimSpace(newModelName) == "" check for model creation
  3. Empty list boundary check ✅ - Added early return when len(filteredItems) == 0 and improved index clamping with if currentIndex >= 0 check
  4. Unnecessary nil assignment ✅ - Removed m.list.SetItems(nil) to prevent potential flickering
  5. Uninitialized variables ✅ - Now using map lookup with foundI, foundJ boolean checks instead of relying on zero values

Let me know if you'd like any further changes!

Copy link
Owner

@sammcj sammcj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for contributing!

@sammcj sammcj merged commit 2f17393 into sammcj:main Dec 30, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

⭐️ Feat: Sorting when using a filter

2 participants

Comments