Add comprehensive filter functionality for changes list #20537
Conversation
- Implement robust filtering for working directory file changes - Add filter state management to app store and repository cache - Update UI components (sidebar, filter-changes-list) for filter support - Enhance dispatcher with filter-related actions - Add comprehensive test suite covering all filter scenarios - Include unit tests and integration tests for filter functionality
|
Why are GH Actions not running on this PR? |
|
@Gr8z Thank you for wanting to contribute. I will bring this up for team discussion And, a few things off hand:
|
|
Hi @tidy-dev, thank you for the detailed feedback. Regarding the issue requirement - You're absolutely right. I should have first opened an issue to discuss the need and gather feedback on whether this feature would be valuable to the user base. I'll create an issue to start that discussion. Thank you for the clarification about GitHub Desktop's staging behavior. I'll revise the terminology to use "Included in commit"/"Excluded from commit". Filter approach—I did not go for exclusion; I think the screenshot might be misleading. The default state is all unchecked. Filters can be included when you select them, and the condition for multiple filters is "OR." That means if I select Deleted and Modified filters, it will show me Deleted "OR" modified files. However, I agree that this assumption should be validated with user research. We should understand:
Would you recommend:
I'm happy to take either approach and appreciate your guidance on making this contribution more aligned with the project's needs. |
Yes. Pausing for now as I do not want to utilize your time on something that will likely take quite some time to garner feedback. Once I discuss it with my team, I will update on if we think we might move forward with it soon or it should be closed until more user feedback is obtained. Thank you for being so understanding. :) |
|
@Gr8z We discussed this today in a team sync and filtering by file status is one of the iterations that we had considered being a next iteration as it was requested in the original filtering changes issue. Thus, there is prior intention for this filter to be added. We may like to move forward with, but have tabled it for further discussion in an upcoming sync in mid June. Thus, I moved it to draft instead of closing it. Things we know without in depth review:
I will leave it up to you if you want to work on those with out a solid yes. |
|
Thank you @tidy-dev, I'm happy to work on the suggested improvements while your team continues the discussion:
I'll start working on these changes to have them ready for when you guys make a final decision in mid-June. |
- Update terminology from "staged/unstaged" to "included in commit/excluded from commit" - Add clear filters functionality with buttons in dropdown and empty state - Implement comprehensive metrics tracking for all filter operations - Enhance no results message with bulleted list of active filters - Add responsive empty state design with proper container queries - Fix hidden changes warning to work with all filter types - Add extensive test coverage for clear filters functionality - Verify OR logic implementation in filter operations This completes PR desktop#20537 with full feature implementation and test coverage.
|
Hi @tidy-dev I have added the changes that you requested. Please take a look now.
Screenshots
|
There was a problem hiding this comment.
Thank you for all your work thus far. 💖 This is generally looking good.
Lots of feedback for you for my initial review on the logical "OR" approach. Feel free to ask questions if anything isn't clear.
FYI, going to gather some feedback from a coworker on the unit tests as they just did a bunch of work in that realm
app/src/lib/stats/stats-store.ts
Outdated
| @@ -242,6 +242,11 @@ const DefaultDailyMeasures: IDailyMeasures = { | |||
| previewedPullRequestCount: 0, | |||
| typedInChangesFilterCount: 0, | |||
| appliesIncludedInCommitFilterCount: 0, | |||
| appliesExcludedFromCommitFilterCount: 0, | |||
| appliesNewFilesFilterCount: 0, | |||
There was a problem hiding this comment.
Just for future proofing, Can we add the word Changes in from of Filter for all these files status stats since there are files lists elsewhere in the app. e.g. appliesNewFilesChangesFilterCount
.. The Include/Exclude from commit should be unique enough since that is something you do in the changes list.
I am waiting for a decision on the AND and OR discussion for me to add those changes (or not). |
There was a problem hiding this comment.
I was asked to review the tests in this PR and the sheer volume of tests here made it impossible for me to go through them all but I've skimmed through a lot of them and I see a lot of the same patterns so I've reviewed the patterns and not each test so I apologize if I've missed something of substance but the tests that I've looked at are all self-contained in so far that the only way they could break is if we changed the tests themselves.
|
|
@tidy-dev do you have any more updates to move this forward? |
There was a problem hiding this comment.
@Gr8z Sorry it took me a while to get back to you!
There are few things we haven't addressed from some of my previous feedback. I apologize for not summarizing it in one place. I decided to see if I could wrap that up real quick for you so we can get this merged and found a few places we can make the code a little cleaner/more efficient Take a look and see if you agree with the changes and pull them in if so. :)
Things it does previously discussed or inferred:
- Makes the filter options count reflect the number of changes that are currently in the list that match the filter. (respecting the AND logic) (So that when you apply a filter the list will narrow to the number shown)
- Makes a couple of the methods not have a redundant
filterTextparameter since that is now on the filter state object. - Removes the redundant get fileFilterState method in favor of directly referring to the props since there is no mutation of the props state happening.
In addition,
- Consolidated the count methods into a single memoized method such that we don't need to loop through all the filtered values for every count metric as they should all be recounted anytime the filtered values or working directory changes.
- I moved the filter popover logic into it's own component to group it's logic together. (Not necessary, just seems easier to digest to me and possibly something I should have done originally when introducing the popover)
- Added some helper methods on to the FileChange classes to make the filter logic a little cleaner.
|
Nice @tidy-dev, took a look at your changes and it looks super clean. I apologize if I missed any of your comments or misinterpreted them. One thing, though, I am not sure if showing the count this way is the right way to go. The filter option counts should show the independent counts, not the combined AND counts.
I have two files here to be committed, but you can't tell because the count is 0. |
I can see where you are coming from. And, I view them as filters and I think the number should help the user use the filter. Just like on any e-commerce site, if you put a search term in, and you receive 100 results. There are additional filters usually provided such as color or rating. Those filters often tell you of the 100 results present 60 are blue and 50 have 4 star rating. But.. if I apply the blue filter, now my list has 60 items and now the ratings counts are updated to say there are 10 items with 4 star ratings in the list (that are blue and match the search term). If we flip this and leave them as independent counts, say I have 100 files in the list and 50 are new, 30 are modified, and 20 are deleted. 50 of them are checked and 50 are not. I open the filter and see those counts. I apply the "New files" filter. The lists filters down to the 50 files. I open the filter options again and I still see 50 are checked and 50 are not checked.. but there are only 50 files in the list.. so I have no idea how many are going to show if I checked "Included in commit". Where as if the filter options inform you of how many files the filter will filter to, when I opened it the second time it should look like "New files" (50), modified/deleted (0), and could look something like "Included in commit" (10) and "Excluded from commit" (40). Now I know if I apply the modified or deleted filter, there will be 0 left in the list and if I apply the "Included in commit" there will be 10 left in the list. Additionally, that is the way the filter works today, if you have 100 files in the list and apply a "test" string then open the filter options the "Included in commit" count will be something < 100 even if all 100 were checked prior to the search string. I propose for now we keep the logic inline with the first iteration and see what kind of community feedback we get for it. Another iteration could be completely switching this logic or could be something like having a (0/100) where the first number shows the filter applied and the second number is the independent count. We do this for the string filter. |
|
The e-commerce analogy misses a key difference in user intent. In e-commerce, users refine searches to shrink choices and discover products from an unknown catalog. In Git clients, users aren't discovering files - they're verifying and organizing work they've already done. They need transparency about repository state to maintain accurate mental models, not guidance on what subset to focus on. When I see "Included in commit (0)" but know I have staged files, it's unclear whether the filter is working correctly, there's a bug, or I've misunderstood the interface. Independent counts would show "Included in commit (2)" regardless of other filters, making it clear that 2 files are staged and the current 0 result is due to filter intersection. The dual format "Included in commit (0/2)" could provide both contextual and absolute info. is there a beta testing branch with an enough user pool to test both approaches and gather actual feedback before committing to either pattern? |
That is my analogy. Using the filter to shrink (filter) choices to smaller subset to commit.. or just find a file to look at in the diff. This component is a tool to accomplish filtering not about summarizing what they already know. Tho, I do agree that it is not perfect analogy as you note about unknown total list versus a finite known list. I revert back to the purpose of the list. The purpose is to allow users to either (1) select a file to review the diff or (2) check files to include in the commit. The filter was made to assist in those purposes when the list is long enough to be hard to scan or you must scroll to find a file. Thus, it purpose is to narrow the list.. not provide ancillary information about the list.
Again to your point, I can see someone using the ancillary information as form of confirmation that the list reflects what they expect it to before they commit so it can also be useful in assisting in commit files. The reason I am hesitant in implementing this solution now is a prior design discussion my team held where we considered it on the first iteration, but chose to limit the design to necessary information for the filter function instead of more numbers to figure out what they represent as the space is too limited to really communicate what they represent.
Great idea! Check out the |
|
I completely understand your perspective, and I trust your judgment. The space constraints you mention are also valid concerns. Either we can implement a feature flag and wait, or, since, in my opinion, this isn't a disruptive feature, users can choose not to use it or completely hide it from view. We can then merge it as is and get fast feedback. |
I agree. If you like to just pull the changes in from my commit, I will take another look and maybe we can get this merged. :) |
- Make counts reflect "AND" logic - Simply some methods that can just consume the filter state
Replaces direct property checks and enum comparisons with method calls on the FileChange object for filtering changes. This improves encapsulation and readability by delegating status checks to the change object's methods.
|
Sounds good, Done! |
There was a problem hiding this comment.
Working as expected. ✨ Super close to merge, but build is failing due to test relying on a object cast instead of instantiating the class as well as a linter error. I have added suggestions to fix the unit test error and then should just need to run yarn lint:fix to address the formatting issue.
|
Updated the tests, they are all passing now @tidy-dev |
Description
This pull request introduces a new feature to filter changes in the repository by file type and status (new, modified, deleted, unstaged, and staged files). The most important changes include updates to the state management, UI components, and dispatcher logic to support these new filters.
State Management Updates:
filterNewFiles,filterModifiedFiles,filterDeletedFiles,filterUnstagedFiles) toIChangesStateand initialized them ingetInitialRepositoryState. [1] [2]createStatehelper in tests to include the new filter properties.UI Enhancements:
FilterChangesListcomponent to include checkboxes for the new filters, dynamically calculate filter counts, and display the number of active filters. [1] [2] [3] [4]ChangesSidebarto pass the new filter states to theFilterChangesListcomponent.Dispatcher and Store Logic:
AppStoreandDispatcherto handle updates to the new filter states (_setFilterNewFiles,_setFilterModifiedFiles, etc.). [1] [2]These changes collectively enhance the user experience by allowing more granular control over the displayed changes in the repository.
Screenshots
Release notes
Notes: Added comprehensive filter functionality for the changes list, allowing users to filter files by status (modified, new, deleted, untracked) for better repository management.