feat(filters): add reusable file type and date filters - #4107
Conversation
WalkthroughAdds localized date and file-type filters with centralized options, icons, selection handling, and clearing. Extends the shared Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
BundleMonUnchanged files (18)
Total files change +3B 0% Groups updated (1)
Unchanged groups (2)
Final result: ✅ View report in BundleMon website ➡️ |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Filters/FileTypeFilter.jsx`:
- Around line 47-49: Update handleChange in FileTypeFilter so selecting the
mobileOptions reset entry with an empty value invokes onClear instead of
onChange. Preserve the existing onChange behavior for non-empty file type
values, ensuring the reset clears the active filter state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 55a2429d-0f63-4fdd-a419-15aeeb24bbc8
📒 Files selected for processing (9)
src/components/Filters/FileTypeFilter.jsxsrc/components/Filters/FileTypeFilter.spec.jsxsrc/components/Filters/Filter.jsxsrc/components/Filters/fileTypes.jssrc/components/Filters/index.jssrc/locales/en.jsonsrc/locales/fr.jsonsrc/locales/ru.jsonsrc/locales/vi.json
shepilov
changed the title
| option: { | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| gap: theme.spacing(1) |
There was a problem hiding this comment.
@JF-Cozy we should have u-gap-xx utility classes no?
There was a problem hiding this comment.
Yes we should, someone tried to implement it a loooooooog time ago in cozy-ui. It's not really used, but can helps in some cases, so, yes 👍
There was a problem hiding this comment.
The PR is still open for u-g-half(8px) cozy/cozy-ui#2281
There was a problem hiding this comment.
yes it will be merged asap. But we can go with gap: theme.spacing(1) without using u-classes if you are in a hurry, we didn't have u-gap for 4 years, it's not a big deal :)
There was a problem hiding this comment.
I merged it, so as you want, either you can remove totally this to use u-flex u-flex-items-center u-g-half or go like this
|
|
||
| export function findFileTypeOption(value) { | ||
| return FILE_TYPE_OPTIONS.find(option => option.value === value) ?? null | ||
| } |
There was a problem hiding this comment.
May we can mutualize something with src/lib/getMimeTypeIcon.jsto avoid multiplying type to icon map?
There was a problem hiding this comment.
yep, I think we can't use it as is, becouse we don't have but we can extract category-to-icon map
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
17bd462 to
746a4ed
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Filters/DateFilter.jsx`:
- Around line 54-64: Update handleChange in DateFilter so selecting the blank
mobile option routes through onClear instead of calling onChange with an empty
string; continue passing known date-range values to onChange unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 416aa962-5438-4dd1-8f7b-b798aee7392c
📒 Files selected for processing (12)
src/components/Filters/DateFilter.jsxsrc/components/Filters/DateFilter.spec.jsxsrc/components/Filters/FileTypeFilter.jsxsrc/components/Filters/FileTypeFilter.spec.jsxsrc/components/Filters/Filter.jsxsrc/components/Filters/fileTypes.jssrc/components/Filters/index.jssrc/lib/getMimeTypeIcon.jssrc/locales/en.jsonsrc/locales/fr.jsonsrc/locales/ru.jsonsrc/locales/vi.json
🚧 Files skipped from review as they are similar to previous changes (7)
- src/locales/en.json
- src/components/Filters/FileTypeFilter.spec.jsx
- src/components/Filters/index.js
- src/locales/ru.json
- src/lib/getMimeTypeIcon.js
- src/components/Filters/fileTypes.js
- src/components/Filters/Filter.jsx
| const mobileOptions = [ | ||
| { | ||
| label: t('filters.date.label'), | ||
| value: '' | ||
| }, | ||
| ...options | ||
| ] | ||
|
|
||
| const handleChange = event => { | ||
| onChange(event.target.value) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Route the mobile placeholder through onClear.
Selecting the blank mobile option currently invokes onChange(''), although value only accepts a known date range (or null). This can leave the parent with an invalid empty-string state instead of clearing the filter.
Proposed fix
const handleChange = event => {
- onChange(event.target.value)
+ const nextValue = event.target.value
+ if (nextValue === '') {
+ onClear()
+ return
+ }
+ onChange(nextValue)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mobileOptions = [ | |
| { | |
| label: t('filters.date.label'), | |
| value: '' | |
| }, | |
| ...options | |
| ] | |
| const handleChange = event => { | |
| onChange(event.target.value) | |
| } | |
| const mobileOptions = [ | |
| { | |
| label: t('filters.date.label'), | |
| value: '' | |
| }, | |
| ...options | |
| ] | |
| const handleChange = event => { | |
| const nextValue = event.target.value | |
| if (nextValue === '') { | |
| onClear() | |
| return | |
| } | |
| onChange(nextValue) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Filters/DateFilter.jsx` around lines 54 - 64, Update
handleChange in DateFilter so selecting the blank mobile option routes through
onClear instead of calling onChange with an empty string; continue passing known
date-range values to onChange unchanged.
746a4ed to
db240c3
Compare
shepilov
changed the title
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
3 participants
Summary
Summary by CodeRabbit