Skip to content

fix: DH-22976 ui.table filters to update when changed programmatically#1377

Open
SimonVutov wants to merge 5 commits into
mainfrom
feat-DH-21841-fix-user-to-modify
Open

fix: DH-22976 ui.table filters to update when changed programmatically#1377
SimonVutov wants to merge 5 commits into
mainfrom
feat-DH-21841-fix-user-to-modify

Conversation

@SimonVutov

@SimonVutov SimonVutov commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

sorts / quick_filters are server-owned: the server controls the value, so updating the prop re-applies it and replaces whatever the user changed in the UI.

New default_sorts / default_quick_filters are user-owned: they set the initial value, then the user owns it from there — their changes are persisted and restored on reload. Use one or the other for a given facet, not both.

@SimonVutov SimonVutov self-assigned this Jun 26, 2026
@github-actions

Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@SimonVutov SimonVutov force-pushed the feat-DH-21841-fix-user-to-modify branch from 2e9c1ae to 8961441 Compare June 29, 2026 13:05
@github-actions

Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@SimonVutov SimonVutov force-pushed the feat-DH-21841-fix-user-to-modify branch from 8961441 to 8eba401 Compare June 29, 2026 13:11
@github-actions

Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@SimonVutov SimonVutov requested a review from mofojed June 29, 2026 13:15

@mofojed mofojed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does allow the user to change the sorts, which is great. However, those changed sorts do not persist as they should (as described in my comment on #1358 (comment)).

I think to fix this properly (for both this case and the filters case described in DH-22976 that I've also assigned to you), we'll need changes in IrisGrid to handle this correctly. I think. I have a snippet specifically for sorting on #1358 (review)

@github-actions

Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@SimonVutov SimonVutov requested a review from mofojed June 30, 2026 14:25
@SimonVutov SimonVutov changed the title fix: DH-21841 Allow user to modify table after ui.table sets sort fix: DH-22976 ui.table filters to update when changed programmatically Jun 30, 2026
@github-actions

Copy link
Copy Markdown

ui docs preview (Available for 14 days)

1 similar comment
@github-actions

Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@mofojed mofojed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add e2e test for this guy to make sure of the following:

@SimonVutov SimonVutov requested a review from mofojed July 2, 2026 13:07
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@SimonVutov

Copy link
Copy Markdown
Contributor Author

Had to re-run the e2e tests a few times.

Summary:

  • Attempt 1 failed with a Chromium ag_grid screenshot mismatch, which looks unrelated to this PR since this PR does not touch the AgGrid test/code.
  • Attempt 2 failed on the new WebKit ui_table persistence test at the screenshot after applying the quick filter. The failure was a screenshot diff.
  • Attempt 3 passed. It still reported a few flaky ui_table screenshot tests, but those passed on retry within the run.

@mofojed mofojed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to work correctly for filtering, using the snippet from the ticket:

from deephaven import ui
import deephaven.plot.express as dx

_stocks = dx.data.stocks()

t = _stocks.where("Sym = `CAT`") # Applied when query is run

@ui.component
def table_filter_picker():
    exchange, set_exchange = ui.use_state("NYPE")
    return ui.flex(
        ui.picker("NYPE", "PETX", on_selection_change=set_exchange, selected_key=exchange, label="Exchange"),
        ui.heading(exchange),
        ui.table( # Filters applied when table is opened on the client
            _stocks,
            show_quick_filters=True,
            quick_filters={
                "Sym": "CAT",
                "Exchange": exchange,
                "Price": ">=100"
            }
        ),
        direction="column"
    )

t2 = table_filter_picker()

Changing the dropdown does not update the filters

// re-applied whenever their value changes. We stabilize them by content so an
// unrelated re-render (new reference, identical content) does not re-apply and
// replace changes the user made in the UI.
const stableSorts = useIsEqualMemo(sorts, deepEqual);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be necessary - we use patching in the deephaven.ui render so it should be a stable object unless there's actually changes. If that's not the case, there's something else that needs to be fixed in the renderer.
We added patching in: #1313

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants