omsn2 commented on PR #43108: URL: https://github.com/apache/superset/pull/43108#issuecomment-5480335416
> Tagging @kgabryje @michael-s-molina and @villebro on this one, since they might have the most institutional memory around native filter state/storage, and remember reasons we didn't do this before. Seems harmless as long as it's non-breaking for others dpeending on filter state persisted in the key/value store and retrieved via the filter key in the URL. If this is just additive like caching, it's probably a nice addition. > > Without having looked at the code yet, I'll note "some does it/should it" questions: • If a dashboard is loaded with no url filter key, and it applies the filters, does/should it ADD the filter key to the URL via push state? One of the benefits of having it there is that you can share that URL with others, so they can benefit from that filter config. Also, just a "truthiness" thing to show more accurately that these filters are applied • If someone sends you a URL with a key in the URL, does local storage USE those filters, or override them with your local filters? • If you keep editing filters locally, and the URL updates the filter key accordingly, are all URL and navigation aspects preserved (i.e. does hitting back go to the prior set values), or does this change behavior? • Are there any situations where the filter state stored locally might be obsolete compared to the data/filters available as fetched from the server in the normal workflow? I.e. might you be applyin g a filter on a column/value that no longer exists in the DB? Of course the normal native filter key might share this problem if it is indeed a problem. • Could any of the bug/feature as it exists today be improved by additional push state entries to make navigation more sensible? > > This also makes me think of a new feature... just since my brain is on this, I'll mention it... when we apply filters on a dashboard, we get the lovely Apply button that generates a native filters key in the URL. Maybe (if you're one of the dashboard owners, or a use with the permission to do so, which is not always the case) we should have a "Save filters as defaults" button near Apply... that cuold save a lot of time for dashboard authors compared to editing the filters and setting default values one by one. > > [edit: this might be the longest review comment I've written as a typing-human in MONTHS! Thanks for a thought-provoking PR] Thanks for the thorough review @rusackas! These are all good questions. I went through the current flow to see how each case behaves today: **Q1 — If there’s no URL filter key, should restoring filters add one to the URL?** Yes, this already happens as part of the existing flow. When `hydrateDashboard` restores the `dataMask`, it updates the Redux state. `FilterBar/index.tsx` then picks up the change through its `useEffect` and calls `publishDataMask`. That eventually calls `createFilterKey` and `history.replace(...)`, which adds the `native_filters_key` to the URL. So after the filters are restored, the URL reflects the active filter state and can be shared with someone else, just like when filters are applied manually. One existing limitation is that `publishDataMask` only runs this way for authenticated users. Embedded/guest users won't get the URL key, but that's existing behavior and isn't introduced by this PR. **Q2 — What happens if both the URL and localStorage have filter state?** The URL takes precedence. The restore logic is essentially: ```text permalinkKey → use permalink state else nativeFiltersKey → use URL filter state else → fall back to localStorage ``` So localStorage is only used when there isn't already an explicit filter state in the URL. If someone shares a URL containing a filter key, that state will be used regardless of what filters the recipient has stored locally. **Q3 — What about Back/Forward navigation when filters change?** This behavior is unchanged by this PR. `publishDataMask` currently uses `history.replace(...)`, rather than `history.push(...)`, so changing/applying filters doesn't create separate browser history entries today. The Back button therefore takes you back to the previous page rather than through previous filter states. I agree that using `push` for explicit user Apply actions could make navigation more intuitive, while keeping `replace` for programmatic changes such as restoration. I think that would be a useful follow-up, but it's outside the scope of this PR. **Q4 — Could the locally stored state become stale?** There is some protection against this already. The restore logic checks the stored filter IDs against the current `native_filter_configuration`, so if a filter has been deleted, it won't be restored. What we don't currently validate is whether the stored `extraFormData` is still valid — for example, if a filter's column or dataset target has changed since the state was stored. In that case, the stale state could potentially result in no/wrong data. This is also a limitation of the existing server-side `native_filters_key` flow, which stores the `extraFormData` without schema-level invalidation, so I don't think this introduces a new regression. A deeper validation/invalidation mechanism could be handled separately if needed. **Q5 — Could additional history entries make navigation better?** Yes, I think so. As mentioned above, using `history.push(...)` for explicit Apply actions could allow users to navigate back through previous filter states. I'd treat that as a separate improvement to the existing `publishDataMask` behavior rather than something specific to this PR. **Regarding the "Save filters as defaults" idea:** I really like this idea. A **"Save as defaults"** action for dashboard owners/users with the appropriate permissions could save the current `dataMask` as the filters' default state, instead of requiring authors to configure each filter's default individually. It seems like it could fit nicely with the existing `native_filter_configuration` structure. I'd be happy to open a separate issue/feature request for this if that's useful. Thanks again for the questions — they helped surface some useful follow-up ideas around filter state and browser navigation. > Thanks for the thorough review @rusackas! These are all good questions. I went through the current flow to see how each case behaves today: **Q1 — If there’s no URL filter key, should restoring filters add one to the URL?** Yes, this already happens as part of the existing flow. When `hydrateDashboard` restores the `dataMask`, it updates the Redux state. `FilterBar/index.tsx` then picks up the change through its `useEffect` and calls `publishDataMask`. That eventually calls `createFilterKey` and `history.replace(...)`, which adds the `native_filters_key` to the URL. So after the filters are restored, the URL reflects the active filter state and can be shared with someone else, just like when filters are applied manually. One existing limitation is that `publishDataMask` only runs this way for authenticated users. Embedded/guest users won't get the URL key, but that's existing behavior and isn't introduced by this PR. **Q2 — What happens if both the URL and localStorage have filter state?** The URL takes precedence. The restore logic is essentially: ```text permalinkKey → use permalink state else nativeFiltersKey → use URL filter state else → fall back to localStorage ``` So localStorage is only used when there isn't already an explicit filter state in the URL. If someone shares a URL containing a filter key, that state will be used regardless of what filters the recipient has stored locally. **Q3 — What about Back/Forward navigation when filters change?** This behavior is unchanged by this PR. `publishDataMask` currently uses `history.replace(...)`, rather than `history.push(...)`, so changing/applying filters doesn't create separate browser history entries today. The Back button therefore takes you back to the previous page rather than through previous filter states. I agree that using `push` for explicit user Apply actions could make navigation more intuitive, while keeping `replace` for programmatic changes such as restoration. I think that would be a useful follow-up, but it's outside the scope of this PR. **Q4 — Could the locally stored state become stale?** There is some protection against this already. The restore logic checks the stored filter IDs against the current `native_filter_configuration`, so if a filter has been deleted, it won't be restored. What we don't currently validate is whether the stored `extraFormData` is still valid — for example, if a filter's column or dataset target has changed since the state was stored. In that case, the stale state could potentially result in no/wrong data. This is also a limitation of the existing server-side `native_filters_key` flow, which stores the `extraFormData` without schema-level invalidation, so I don't think this introduces a new regression. A deeper validation/invalidation mechanism could be handled separately if needed. **Q5 — Could additional history entries make navigation better?** Yes, I think so. As mentioned above, using `history.push(...)` for explicit Apply actions could allow users to navigate back through previous filter states. I'd treat that as a separate improvement to the existing `publishDataMask` behavior rather than something specific to this PR. **Regarding the "Save filters as defaults" idea:** I really like this idea. A **"Save as defaults"** action for dashboard owners/users with the appropriate permissions could save the current `dataMask` as the filters' default state, instead of requiring authors to configure each filter's default individually. It seems like it could fit nicely with the existing `native_filter_configuration` structure. I'd be happy to open a separate issue/feature request for this if that's useful. Thanks again for the questions — they helped surface some useful follow-up ideas around filter state and browser navigation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
