Repository navigation
Fixes #35536: Add and edit custom metrics in one drawer - #35493
ShaileshParmar11 wants to merge 15 commits into
Conversation
github-actions
Bot
added
safe to test
Replace the standalone /add-custom-metric page and the edit modal with a single CustomMetricFormDrawer hosted by TableProfilerProvider. The profiler Add menu opens it for a new table/column metric and the metric graph's Edit action opens it prefilled. The drawer keeps the page's profile preview (table profile charts, or the selected column's profile) beside the form, rendered outside the profiler context so its metric graphs stay read-only. Duplicate-name validation now follows the selected column instead of the URL.
The former add page navigated back to the metric's column; without that the drawer left users on the column list, where the new graph is not shown.
f2ebeef to
ca44e77
Compare
…om-metric-form-drawer
…olumn Shrink the preview to two fifths of the drawer and let the profile widgets reflow there: the column summary and Data Quality Tests cards stack when their container is narrow (container query, so the page layout is unchanged), latest-value metrics wrap instead of breaking labels per character, and the no-profiler message wraps instead of overflowing.
The column summary, Data Quality Tests, profiler details and state wrapper cards had no background and borrowed the page's white card. On the custom metric drawer's grey preview column they turned grey; tw:bg-surface keeps them white there and follows the card surface in dark mode.
… preview A 24-column grid's 23 gaps (460px) are wider than the drawer's preview column, so the stacked cards spilled past it and the panel scrolled sideways. Use a single-column grid until the container is wide enough for the 24-column side-by-side layout.
Swap SchemaEditor for CodeEditor so the custom metric SQL Query field matches the test case drawer's SQL Expression box: copy button header, no line-number gutter.
…d add page The impact map still pointed at pages/AddCustomMetricPage/**, which no longer exists, so changes to the drawer would not select CustomMetric.spec.ts.
…core components Per review: the column summary and Data Quality Tests cards now use the core Card (default style, Card.Header/Card.Content) instead of hand-rolled bordered divs, their row is a wrapping core Box that stacks when the container is too narrow for two 20rem cards (replacing the container query), and the custom metric drawer lays out its form and preview with core Grid/Grid.Item.
Per review: ProfilerDetailsCard and ProfilerStateWrapper drop their hand-rolled bordered divs (custom 10px radius) for the core Card with Card.Content, so the profiler cards share the design system's radius, border and surface. The cards stay overflow-visible so chart tooltips near the edge are not clipped.
…ontent and Box Drop the custom padding and flex classes on the latest-value card for Card.Content's default padding and core Box layout, matching the other profiler cards.
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
|
| Count | Rule |
|---|---|
| 16 | react-hooks/exhaustive-deps |
| 2 | openmetadata-imports/no-lower-layer-page-imports |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/DataQuality/CustomMetricForm/CustomMetricFormDrawer.tsx:91:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'activeColumnFqn', 'form', and 'table?.columns'. Either include them or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/DataObservability/TabFilters/TabFilters.tsx:78:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has an unnecessary dependency: 'isTourOpen'. Either exclude it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/DataObservability/TabFilters/TabFilters.tsx:108:9 |
react-hooks/exhaustive-deps |
The 'handleTestCaseClick' function makes the dependencies of useMemo Hook (at line 139) change on every render. Move it inside the useMemo callback. Alternative |
| 🟡 | src/components/Database/Profiler/DataObservability/TabFilters/TabFilters.tsx:113:9 |
react-hooks/exhaustive-deps |
The 'handleCustomMetricClick' function makes the dependencies of useMemo Hook (at line 139) change on every render. Move it inside the useMemo callback. Alterna |
| 🟡 | src/components/Database/Profiler/TableProfiler/CustomMetricGraphs/CustomMetricGraphs.component.tsx:119:5 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/SingleColumnProfile.tsx:152:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'profilerDocsLink' and 't'. Either include them or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/SingleColumnProfile.tsx:224:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'createMetricsChartData'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/TableProfilerChart/TableProfilerChart.tsx:133:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'profilerDocsLink' and 't'. Either include them or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/TableProfilerChart/TableProfilerChart.tsx:154:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has an unnecessary dependency: 'profileMetrics'. Either exclude it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/TableProfilerChart/TableProfilerChart.tsx:192:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'fetchProfilerData'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/TableProfilerProvider.tsx:213:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 'getProfileSampleValue'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/TableProfilerProvider.tsx:238:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has an unnecessary dependency: 'allTestCases'. Either exclude it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/TableProfilerProvider.tsx:354:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'fetchLatestProfilerData', 'isTableDeleted', and 'tableProfiler'. Either include them or remove the dependency ar |
| 🟡 | src/components/Database/Profiler/TableProfiler/TableProfilerProvider.tsx:366:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'fetchAllTests'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/TableProfilerProvider.tsx:392:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'fetchTestCaseSummary'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/TableProfilerProvider.tsx:417:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 'fetchAllTests'. Either include it or remove the dependency array. |
| 🟡 | src/utils/RouterUtils.ts:47:1 |
openmetadata-imports/no-lower-layer-page-imports |
Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here. |
| 🟡 | src/utils/RouterUtils.ts:48:1 |
openmetadata-imports/no-lower-layer-page-imports |
Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here. |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed
Jest test CoverageUI tests summary
|
The drawer rendered outside TableProfilerContext, so its profile preview read an empty context: the column's Data Quality Tests showed 0/0/0 and the charts showed the set-up-profiler message even when profiling was enabled. Wrap the drawer in a copy of the profiler context with permissions cleared. The preview now reads testCaseSummary, isProfilingEnabled and the custom metrics, while CustomMetricGraphs (Edit/Delete only when permissions is set) and TableProfilerChart (blocks only when permissions lack ViewDataProfile) stay read-only. permissions becomes optional on the context interface; every consumer already guards it.
ShaileshParmar11
changed the title
Code Review ✅ Approved 1 closed / 1 findings🟡 Medium risk · Moving custom metric creation and edits into a shared drawer changes persisted SQL expressions; a mistaken edit could distort reported metrics. Custom metric drawer now wraps the profile preview in a read-only copy of the profiler context with permissions cleared, so Data Quality Tests and the empty state match the profiler page. No open issues remain. ✅ 1 closed✅ Bug: Drawer preview outside provider shows 0 tests and wrong empty state
VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
Describe your changes:
Fixes #35536
Custom metrics are now added and edited in a single drawer. The standalone
/add-custom-metric/:dashboardType/:fqnpage and the edit modal are gone, so there is one way to create or change a custom metric. The drawer keeps the page's documentation panel: the table profile charts for a table metric, and the selected column's profile for a column metric. It follows #35190 (merged), which moved Classic Observability off Ant Design.Type of change:
High-level design:
TableProfilerProviderrenders a lazyCustomMetricFormDrawer, the same way it hosts the test case drawer, and exposesonCustomMetricDrawerOpen(isColumnMetric, metric?)on the profiler context.TabFilters) opens it empty.CustomMetricGraphs) opens it prefilled, with name and column locked.Grid: the form (CustomMetricForm, unchanged API) takes 14 of 24 columns, and the profile preview takes 10:TableProfilerChart, orSingleColumnProfilefor the column picked in the form.permissionscleared) instead of none. The preview readstestCaseSummary,isProfilingEnabledand the custom metrics, so Data Quality Tests and the empty state match the profiler page.CustomMetricGraphsonly offers Edit/Delete whenpermissionsis set, andTableProfilerChartonly blocks whenpermissionslacksViewDataProfile, so the preview stays read-only.permissionsis now optional on the context type; every consumer already guarded it.tw:z-40, because the coreSlideoutMenuoverlay has no z-index and the profiler sub-tabs painted through it. The test case drawer has the same fix in LESS.PUTresponse, with no page reload.activeColumnFqnin the URL.SchemaEditorto the coreCodeEditor(copy button header, no line numbers), matching the test case form's SQL Expression.ProfilerDetailsCard,ProfilerLatestValue,ProfilerStateWrapper,ColumnSummaryand the Data Quality Tests card now use the coreCard,BoxandGridinstead of hand-rolled bordered divs. This intentionally changes padding, radius (now the core 12px) and header size on the Table and Column Profile pages, not only in the drawer. The column summary and Data Quality Tests cards sit side by side on the page and wrap to a stack in narrow containers such as the drawer preview; latest-value metrics wrap instead of breaking labels per character, and the no-profiler message wraps instead of overflowing.AddCustomMetricPageand its test,ROUTES.ADD_CUSTOM_METRIC,getAddCustomMetricPathandPLACEHOLDER_DASHBOARD_TYPE. Collate's own code does not reference any of them..github/playwright/impact-map.jsonnow mapscomponents/DataQuality/CustomMetricForm/**instead of the removed page.Tests:
Use cases covered
PUTkeeps persisted fields but notid.Unit tests
CustomMetricFormDrawer.test.tsx, covering create, column preselect and preview, navigation after a column create, the edit payload, the error path and duplicate names. TheTableProfilerProvider.test.tsxdrawer test covers open, close, add vs edit, and the read-only context (the drawer sees the provider-computed test summary and profiling state, andpermissionsis undefined).CustomMetricGraphs.test.tsx(real Edit → drawer),TabFilters.test.tsx(Add → drawer for table and column),SingleColumnProfile.test.tsx,TableProfilerChart.test.tsx,ProfilerStateWrapper.test.tsx.CustomMetricForm,Database/Profiler,RouterUtilsandAuthenticatedAppRouteron3579d983d4b: 29 suites, 416 tests passed.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
playwright/e2e/Features/CustomMetric.spec.tsandplaywright/utils/customMetric.tsfor the drawer flow. They add Edit steps for both table and column metrics, and the large-column Select test now runs in the drawer.npx playwright test playwright/e2e/Features/CustomMetric.spec.ts --project=chromiumon3579d983d4b, against a Docker stack built from the same commit (./docker/run_local_docker.sh -m ui -d mysql, server revision3579d983d4b) with sample data: 6 passed (30.1s), setup and teardown included.Earlier, on the commits before the profiler-card refactor, the wider impacted set (90 tests across CustomMetric, Profiler, ColumnProfileGraphsAfterReindex, NestedColumnsExpandCollapse, AddTestCaseNewFlow, DataQuality, DataQualityPermissions, Dimensionality, TestSuiteMultiPipeline, TableLevelTests, ColumnLevelTests and FailureThreshold) passed; one DataQuality tooltip test was flaky and passed on retry:
Manual testing performed
./docker/run_local_docker.sh -m ui -d mysql), checked/api/v1/system/versionreports revision3579d983d4b, and waited for the sample data DAG to finish.sample_data.ecommerce_db.shopify.dim_address→ Data Observability → Table Profile, used Add → Custom Metric: the drawer showed the form and the table profile preview. Created a metric: toast, drawer closed, graph appeared.address_idactive, used Add → Custom Metric: the column was preselected and previewed. Switched toshop_id: the preview followed, including its Data Quality Tests (1 success), and saving moved the page toshop_id.UI screen recording:
Recorded on
3579d983d4bagainst the Docker stack above (server revision3579d983d4b, MySQL, Elasticsearch, ingestion healthy; repository sample data loaded by the runner). 1600×900, 35s. Shows: Table Profile → Add → Custom Metric → create; Edit from the graph menu (prefilled, name locked); Column Profile → Add withaddress_idpreselected, switching toshop_idwith the preview following, and saving toshop_id.custom-metric-drawer.mp4
Checklist:
Fixes <issue-number>: <short explanation>Fixes #35536above.🤖 Generated with Claude Code
Summary by Gitar
ProfilerLatestValue,ProfilerDetailsCardandProfilerStateWrapperonto coreCardandBoxcomponentsColumnSummaryandSingleColumnProfilelayouts with coreCard,GridandBoxSchemaEditorto coreCodeEditorwith a copy buttonThis will update automatically on new commits.