feat: Add input filter support to GridWidgetPlugin#2438
feat: Add input filter support to GridWidgetPlugin#2438mattrunyon merged 18 commits intodeephaven:mainfrom
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2438 +/- ##
==========================================
+ Coverage 47.21% 47.31% +0.10%
==========================================
Files 718 723 +5
Lines 39620 39772 +152
Branches 9914 10144 +230
==========================================
+ Hits 18706 18819 +113
- Misses 20903 20942 +39
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for input filters within the grid widget plugin and enhances related dashboards and filter event handling. Key changes include adding new hooks (e.g. useDashboardColumnFilters), updating event handling in panels and plugins, and modifying associated tests and type definitions to support the new filter functionality.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/dashboard/src/useDhId.test.tsx | New tests for the useDhId hook behavior |
| packages/dashboard/src/useDashboardId.ts | Added hook for retrieving dashboard IDs via context |
| packages/dashboard/src/layout/LayoutUtils.ts | Updated the PanelId type to remove array support |
| packages/dashboard/src/DashboardLayout.tsx | Wrapped panel components with DhIdContext provider |
| packages/dashboard/src/Dashboard.tsx | Integrated DashboardIdContext and FiberProvider changes |
| packages/dashboard/package.json | Added new type dependency for react-reconciler |
| packages/dashboard-core-plugins/src/useDashboardColumnFilters.ts | New hook to subscribe and filter dashboard column filters |
| packages/dashboard-core-plugins/src/useDashboardColumnFilters.test.tsx | Introduced tests validating new filter hook behavior |
| packages/dashboard-core-plugins/src/GridWidgetPlugin.tsx | Enhanced grid widget to support input filters and process clear filters events |
| packages/dashboard-core-plugins/src/FilterPlugin.tsx | Updated filter event handling with standardized event listener hooks |
| Others | Various panel and linker components updated to wire filter events correctly |
Comments suppressed due to low confidence (2)
packages/dashboard-core-plugins/src/useDashboardColumnFilters.test.tsx:124
- Duplicate test names found in the test suite, which could lead to confusion in test results. Consider renaming one of the tests to clearly distinguish between different test scenarios.
test('Ignores filters with different name or type', () => {
packages/dashboard/src/layout/LayoutUtils.ts:32
- The updated PanelId type now only supports a single string value instead of an array. Verify that all downstream consumers are compatible with this change to avoid potential type mismatches.
export type PanelId = Brand<'PanelId', string | undefined>;
|
|
||
| static getInputFiltersForColumns( | ||
| columns: readonly DhType.Column[], | ||
| columns: readonly { name: string; type: string }[], |
There was a problem hiding this comment.
Hrm. Another one of these types, though we don't want iris-grid depending on dashboard-core-plugins.
We've got out @deephaven/filters package - maybe makes sense to pull the input filter type definitions down and use them in both iris-grid/dashboard-core-plugins?
There was a problem hiding this comment.
This util doesn't even need to be in IrisGridUtils. It's only used by the panel and the new hook which are both in dashboard-core-plugins. I don't see it used anywhere in enterprise
For relatively simple types like this, I'm not opposed to just having it as an inline literal. The util doesn't actually care that it's an input filter column or a table column or a dropdown filter column. It cares that it has those 2 props and all those other types satisfy that constraint
Originally I had all of these as just dh.Column[], but that is not the case when using it within charts. Chart has a FilterColumnMap type that has values { name: string; type: string }
There was a problem hiding this comment.
Yea it probably doesn't really make sense in IrisGridUtils. But no need to make that a breaking change here.
| log.warn( | ||
| `useDhId must be used within a DhIdContext provider if there is no ${DH_ID_PROP} prop. Defaulting to empty string.` | ||
| ); | ||
| return ''; |
There was a problem hiding this comment.
Ehhh I don't like returning an empty string either - should return a string | null, or even two signatures with a param e.g. useDhId(throwOnNull: false): string | null and useDhId(throwOnNull: true): string
In useDashboardColumnFilters we shouldn't be emitting events with a blank or null panelId I don't think.
There was a problem hiding this comment.
The original intent was to throw always on no ID, but that breaks dh.ui without the update to dh.ui to pass through the panel ID
I've just switched to string | null return for now. Although it's annoying using the branded type for FilterColumnSourceId because I have to cast when calling useDhId and changing the return type to string | null didn't cause the original cast to fail.
I am in favor of removing the branded type for FilterColumnSourceId. I don't think it's somewhere we will commonly mix up ID types and they're both strings at the end of the day. The filter doesn't care if it's a panel or widget ID, it just needs a unique identifier
This adds a few new hooks
useDhIdwhich will be used to route to a specific component in dh.ui (via prop), or the panel ID (via context) as a fallback. This usesuseFiberwhich is not exported because it's an escape hatch that should be used very rarely.useDhIdtakes no props and can read the__dhIdprop of the component via theuseFiberhook.useDashboardIdgives the ID of the current dashboarduseDashboardColumnFiltersis used to subscribe to column filter changes and also advertise the columns and table (if provided) available for filtering. This should allow us to hook up the existing filtering/linking to dh.ui and probably alsoChartBuilderin dh.ui tables.The contexts for
useDhIdanduseDashboardIdare added toDashboardso there shouldn't be any changes for enterprise since it uses theDashboardcomponent.Also reworked the events around this to have stronger types using the helper methods we added.
Test with dh.ui single normal table in a panel (ui.table support PR will follow in plugins repo)
Run this. Then add an input filter via the UI and the columns from the table should be filterable in the table