Conversation
There is new button in pull request diff template, which opens dropbox containing list of all unique extensions that the diff contains. List is created with checkmarks that can be checked/unchecked to filter out files with specific extension. There is also "select all" and "deselect all" buttons. To apply the change and filter out the files user have to click on "Apply" button. If any extensions were filtered out, then button which opens the dropbox is outlined to signal some files were filtered out.
…FileExtensionFilter
|
|
Can we also move the filter to the left, above the files and also add the search bar? |
…ng for loading more files and general code cleaning
|
In recent changes I've fixed the code for those issues:
And I also added the search box, as suggested. I did not changed the position of the button, waiting for more voices for this change. |
@silverwind whats your opinion on that? |
bircni
left a comment
There was a problem hiding this comment.
Please do a proper pass over the AI-generated output before requesting review.
| */ | ||
| function getExtension(filename: string): string { | ||
| const lastDot = filename.lastIndexOf('.'); | ||
| if (lastDot === -1 || lastDot === 0) { |
There was a problem hiding this comment.
Files like .gitignore, .eslintrc, .env all map to '(no extension)', mixing them with files that truly have no extension (e.g., Makefile, Dockerfile). A common convention is to treat the full name as the "extension" for dotfiles, or at minimum use a separate label.
There was a problem hiding this comment.
Follow Golang's approach: always include the dot in ext name.
'.zip', '.html', '.env'
| /** | ||
| * Filter extensions based on search query | ||
| * Matches against extension name (e.g., ".ts", ".go") | ||
| */ |
There was a problem hiding this comment.
I documented all the functions by default, cause there were no clues about documenting the functions in the guidelines I belive. Is it a problem?
There was a problem hiding this comment.
If a function is designed clear enough (clear name, clear arguments, clear logic), it doesn't need to duplicate the comment.
Comments are used to explain more than function name itself and provide more useful information.
But TBH, for this function, even after read the comment, I don't know how it works.
I think you can show some examples: "if searchQuery=.., then return ..., otherwise, return ..."
templates/repo/diff/box.tmpl
Outdated
| {{template "repo/diff/whitespace_dropdown" .}} | ||
| {{template "repo/diff/options_dropdown" .}} | ||
| {{if .PageIsPullFiles}} | ||
| <div id="diff-extension-filter" data-filter_by_file_extension="{{ctx.Locale.Tr "repo.pulls.filter_by_file_extension"}}" data-select_all="{{ctx.Locale.Tr "repo.pulls.select_all_file_extensions"}}" data-deselect_all="{{ctx.Locale.Tr "repo.pulls.deselect_all_file_extensions"}}" data-apply="{{ctx.Locale.Tr "repo.pulls.apply_file_extension_filter"}}"> |
There was a problem hiding this comment.
data-filter_by_file_extension
mix of _ and -
| select_all: el.getAttribute('data-select_all') ?? 'Select all', | ||
| deselect_all: el.getAttribute('data-deselect_all') ?? 'Deselect all', | ||
| apply: el.getAttribute('data-apply') ?? 'Apply', | ||
| search: el.getAttribute('data-search') ?? 'Search extensions...', |
There was a problem hiding this comment.
It's just defensive approach to not cause potential TypeScript error, but I do noticed that the other vue component was leaving locale records without defaults.
There was a problem hiding this comment.
No need to be too defensive.
If programming error happens, make it exposed as early as possible, no need to hide faults.
| function getExtension(filename: string): string { | ||
| const lastDot = filename.lastIndexOf('.'); | ||
| if (lastDot === -1 || lastDot === 0) { | ||
| return '(no extension)'; |
| v-model="ext.checked" | ||
| > | ||
| <label :for="`ext-filter-${ext.ext}`" class="tw-cursor-pointer"> | ||
| <span class="tw-font-mono">{{ ext.ext }}</span> |
There was a problem hiding this comment.
No monospace font in non-code please.
| </template> | ||
| </div> | ||
| <div v-if="filteredExtensions.length === 0" class="tw-py-4 tw-text-center tw-text-text-light-2"> | ||
| {{ locale.no_results ?? 'No extensions found' }} |
| } | ||
|
|
||
| onMounted(() => { | ||
| document.body.addEventListener('click', onBodyClick, true); |
There was a problem hiding this comment.
Body listener is too broad. Register listener at a closer parent inside the component.
|
@silverwind @wxiaoguang @bircni I've made edits based on your comments :) |
|
please press on |
I think its better to let the maintainer resolve it themselves in case they may not see something as actually fixed |
|
@lunny Can you share more details, I see you have 7 files in pull request and extension filter is showing two, so I would like to know which other files were not calculated. Or you have something else in mind? |




Add file extension filter for pull request diffs
Introduces a new dropdown filter in the PR diff toolbar that allows reviewers to show/hide files based on their extensions. This helps focus on specific file types during code review (e.g., viewing only
.tsfiles when reviewing TypeScript changes, or hiding.test.jsfiles when reviewing implementation).Closes: Feature of file filter in PR page #27256
Usage
How it works
.tw-hiddenCSS class to non-matching files, instantly hiding themKeyboard Navigation
For users who prefer keyboard navigation:
Technical Details
DiffFileExtensionFilter.vue) following the same pattern as the existing commit filterTesting
Screenshots
Filter by extension dropdown:

State when files are filtered out:

Note: This implementation uses AI assistance (GitHub Copilot). All code has been reviewed and tested against Gitea's contribution guidelines and passes all linting checks.