Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] not clear filters and sorters when clicking menu item #6300

Open
dfang opened this issue Sep 5, 2024 · 9 comments
Open

[BUG] not clear filters and sorters when clicking menu item #6300

dfang opened this issue Sep 5, 2024 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@dfang
Copy link

dfang commented Sep 5, 2024

Describe the bug

In a table, apply some filters then click the menu item in left side bar, the url should reset filters and sorters (only apply default ones)

Steps To Reproduce

  1. go to https://example.admin.refine.dev/orders
  2. apply status filter
  3. click 'Orders' in left side bar

Expected behavior

reset the filters

Packages

    "@ant-design/charts": "^1.4.2",
    "@ant-design/graphs": "^1.4.0",
    "@ant-design/icons": "5.0.1",
    "@dnd-kit/core": "^6.1.0",
    "@dnd-kit/modifiers": "^7.0.0",
    "@dnd-kit/sortable": "^8.0.0",
    "@emotion/react": "^11.11.1",
    "@emotion/styled": "^11.11.0",
    "@googlemaps/react-wrapper": "^1.1.35",
    "@refinedev/antd": "^5.42.0",
    "@refinedev/cli": "^2.16.36",
    "@refinedev/core": "^4.53.0",
    "@refinedev/devtools": "^1.2.6",
    "@refinedev/graphql": "^6.5.4",
    "@refinedev/inferencer": "^4.6.6",
    "@refinedev/kbar": "1.3.12",
    "@refinedev/react-router-v6": "^4.5.11",
    "@refinedev/simple-rest": "^5.0.8",
    "@refinedev/supabase": "^5.9.2",

Additional Context

No response

@dfang dfang added the bug Something isn't working label Sep 5, 2024
@alicanerdurmaz
Copy link
Member

Hey @dfang, thanks for reporting the issue. I will look into this and let you know when I find a solution.

@arndom
Copy link
Contributor

arndom commented Sep 17, 2024

@alicanerdurmaz Can I work on this

@BatuhanW
Copy link
Member

Hey @arndom sure. Assigning to you.

@arndom
Copy link
Contributor

arndom commented Oct 2, 2024

Hello @BatuhanW

While attempting to resolve this issue, I discovered this bug is present even in new refine projects. I checked multiple examples as well and it's the same thing.

Eventually, I found that the issue is coming from the useTable hook in @refinedev/core, as this is what is used to update the URL based on the filters and sorters coming from the tables (Antd, MUI).

How do I go about running the refinedev/core package?

@aliemir
Copy link
Member

aliemir commented Oct 10, 2024

I want to clarify the issue here and propose a small workaround. Refine's syncWithLocation works in one direction and only infers the parameters at initial render to avoid syncing issues with the state and the query params.

  • When useTable is mounted and a routerProvider is present and ready, filters, sorters, search etc. are inferred from query params.
  • Those values are stored in a state in the useTable hook, any changes in those states will update the query params.
  • If there are any changes in the query params that is not caused by useTable and if those changes doesn't re-mount the hook, those changes are not synced with the internal states.
  • Initially, this was done to avoid unnecessary renders. Having a two-way binding will probably require deep equality checks and might be more complex than it seems to properly implement.

We've discussed some changes and refactors about this feature with the team before but it wasn't prioritized. We don't want this issue to be resolved partially and only work for some use cases such as only clearing it when query params are emptied. We'd be happy help if anyone wants to work on a proper implementation with two-way binding 🙏

For now, as a workaround a small effect can be added to the page to check for the changes in the query params and reflect those. Check the example below:

const { params } = useParsed();
const { setFilters, setSorters } = useTable();

React.useEffect(() => {
  // actual implementation might be different and slightly more complex depending on the use case.
  if (!params.filters) setFilters([], "replace");
  if (!params.sorters) setSorters([]);
}, [params]);

@arndom
Copy link
Contributor

arndom commented Oct 12, 2024

Ah ok, having the query changes affect wherever synching is done is a better solution.

Where do I start though? From what I can see the syncWithLocation value after being defined in RefineContext is used in multiple implementations, the useTable hook for example, so when in sync the params changes should be reflected.

This is off the top of my head but does that mean changes would have to be made wherever params synching is needed? Or are there more efficient solutions your team has discussed?

@arndom
Copy link
Contributor

arndom commented Nov 3, 2024

hey @aliemir I finally had some time to work on this. My approach is to use the params from useParsed() as it is always up to date with the URL.

We can then compare the "new params" and the current params from the internal, if there is a match do nothing, otherwise rebuild internal state with the new query params. Possibly something like this in the useTable hook:

const current_sorters = differenceWith(
      sorters,
      preferredPermanentSorters,
      isEqual,
    );

const final_sorters = differenceWith(
      parsedParams.params.sorter,
      current_sorters ,
      isEqual,
    );

For the other implementations that depend on the syncWithLocation variable a similar approach could be used. What do you think?

Copy link

stale bot commented Jan 2, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jan 2, 2025
@arndom
Copy link
Contributor

arndom commented Jan 3, 2025

@aliemir I'm stuck on the issue of fixing the exceeding of the max depth of the useEffect.

  • The process starts with storing the last sync of state params(filters and sorters) in a ref.

  • So in the useEffect, I compare the current URL params and state params.

    • I also compare the last synched state params and the current state params, this ensures that internal changes are done before attempting to compare with the URL params.
  • When there is a mismatch, the URL and state params are merged, if there is no param in the URL, the state is re-initialized.

  • The logic for this URL -> State binding works fine alone.

  • The issue is that this useEffect doesn't properly wait for the pre-existing useEffect that updates the URL params triggered by a state change to be done. Here:

  • I've tried a timeout(which is not optimal) before updating params in the new useEffect but it worked half the time in my tests.

  • Due to this, the URL and State will never match, triggering the useEffect to be continuously called.

  • I've pushed the latest, with my useEffect commented out, I would appreciate some assistance. Here

@BatuhanW BatuhanW removed the wontfix This will not be worked on label Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants