Fix filtered INDEX view not loading #7501
Merged
Conversation
This file contains 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
charlesBochet
commented
Oct 8, 2024
| const setUnsavedViewFilter = useSetRecoilComponentFamilyStateV2( | ||
| unsavedToUpsertViewFiltersComponentFamilyState, | ||
| { viewId: viewIdQueryParam }, | ||
| { viewId: viewIdQueryParam ?? currentViewId }, |
There was a problem hiding this comment.
also, viewIdQueryParam is not always defined. If viewId is in the URL, we use this view as our support, if not we will take the key=INDEX view
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request addresses an issue with filtered INDEX views not loading correctly in the Twenty application. The main changes focus on the QueryParamsFiltersEffect component, resolving an infinite loop problem and simplifying the component's logic.
Key points:
- Removed dependency on useGetCurrentView hook in QueryParamsFiltersEffect.tsx
- Directly loaded currentViewId using Recoil atom to prevent infinite loop
- Eliminated unmounting logic for unsaved filters, deemed unnecessary
- Simplified component logic to fix filtered INDEX view loading issues
- Removed potential redundant state updates, improving performance
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
| const setUnsavedViewFilter = useSetRecoilComponentFamilyStateV2( | ||
| unsavedToUpsertViewFiltersComponentFamilyState, | ||
| { viewId: viewIdQueryParam }, | ||
| { viewId: viewIdQueryParam ?? currentViewId }, | ||
| ); | ||
|
|
||
| const { resetUnsavedViewStates } = useResetUnsavedViewStates(); |
There was a problem hiding this comment.
logic: resetUnsavedViewStates is imported but never used
| if (isDefined(currentViewId)) { | ||
| resetUnsavedViewStates(currentViewId); | ||
| } | ||
| }; | ||
| }, [ | ||
| getFiltersFromQueryParams, | ||
| hasFiltersQueryParams, | ||
| resetUnsavedViewStates, |
There was a problem hiding this comment.
logic: resetUnsavedViewStates is in the dependency array but not used in the effect
thomtrp
approved these changes
Oct 8, 2024
harshit078
pushed a commit
to harshit078/twenty
that referenced
this pull request
Oct 14, 2024
## Context We have recently merged a refactoring of our view module. However, one case was forgotten which is to test our dynamic filtering logic. It is currently possible to pass unsaved filters through the URL and these filters will be applied to the currentView through `QueryParamsFiltersEffect`. This component was saving filters but also listening to them through useGetCurrentView hook. ## How 1) I'm removing this infinite loop by directly loading currentViewId through the right recoil atom. Bonus: I'm also removing the unmounting logic which seems wrong to me as unsaved filters are mounted on a specific view, there is no need to remove them while switching views in my opinion.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Context
We have recently merged a refactoring of our view module. However, one case was forgotten which is to test our dynamic filtering logic.
It is currently possible to pass unsaved filters through the URL and these filters will be applied to the currentView through
QueryParamsFiltersEffect. This component was saving filters but also listening to them through useGetCurrentView hook.How
Bonus: I'm also removing the unmounting logic which seems wrong to me as unsaved filters are mounted on a specific view, there is no need to remove them while switching views in my opinion.