Reflow the app-mode test case details page: result history first, header and banner aligned - #32966
Conversation
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
safe to test
|
| Count | Rule |
|---|---|
| 5 | openmetadata-imports/no-lower-layer-page-imports |
| 1 | react-hooks/exhaustive-deps |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/DataQuality/IncidentManager/TestCaseResultTab/TestCaseResultTab.component.tsx:26: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/components/DataQuality/IncidentManager/TestCaseResultTab/TestCaseResultTab.component.tsx:342:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array. |
| 🟡 | src/components/observability/TestCaseDetail/TestCaseDetail.tsx:33: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/components/observability/TestCaseDetail/TestCaseDetail.tsx:34: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/components/observability/TestCaseDetail/TestCaseDetail.tsx:35: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/components/observability/TestCaseDetail/TestCaseDetail.tsx:40: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
|
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
731fbd5 added `playwright/utils/appMode.ts`, but `playwright/e2e/Utils/appMode.ts` already existed and does strictly more: it seeds the session tuple *and* stubs the boot `GET /users/{id}/preferences`. That second half is load-bearing — `AuthProvider.hydrateAndResolveAppMode` runs before the first render and unconditionally overwrites the seeded tuple with `resolveEffectiveAppMode(userPref, ...)`, so with no server preference the seed is lost and the tab boots Classic. The duplicate also wrote a `source: 'manual'` field that the current `writeAppMode` shape no longer has. So the details-page spec was seeding a tuple that got thrown away, and would have reached the `/observability/*` routes only on an account that already preferred AI mode. Point it at the real helper and delete the copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four tests flip `isTabExpanded` to true to make the right rail visible, but the `afterEach` reset only restored the other mutated store fields, so the flag leaked into every following test. Nothing asserts on it today, which is why the suite still passes — the reset closes the order dependence before it bites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three of the review comments: * `TableClass` moves into `beforeAll`. Its generated entity name is fixed at construction, so a module-scope instance means a retry that reuses the worker re-runs `create` with the same name and gets a 409. Declared with `!` since TypeScript cannot see that `beforeAll` assigns before any test reads it. * `openDetailsPage`'s body moves to `playwright/utils/testCases.ts` as `openTestCaseDetailsPage(page, fqn)`, next to `verifyTestCaseLastRunBanner`. It takes the FQN rather than closing over the spec's `table`, and it does not seed the app mode — that stays in the spec, so the helper works for either mode and `playwright/utils` keeps its one-way dependency on `playwright/e2e` (no util imports from `e2e/` today). * All five `test.slow()` calls are gone. The per-test timeout is 60s (`playwright.config.ts:517`) and each test is a single navigation plus assertions, so tripling it was blanket application — which is what the rule about `test.slow()` belonging only in the test that needs it is for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
bd0295d moved `TableClass` construction into `beforeAll` but the edit that added the assignment silently did not apply, so `table` stayed `undefined` and `table.create` threw for every test in the spec. The `!` added in that same commit is what hid it — a definite-assignment assertion tells TypeScript to stop checking, so `tsc` had nothing to report. Caught by review on #32966. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Conflicts came from #32982, which added a class-base gate on the shared test-result graph while this branch moved that same graph to the top of the main column. * `TestCaseResultTab.component.tsx` — the graph keeps this branch's position and takes main's gate: `shouldRenderTestSummary(testCaseData, shouldRenderDefaultGraph)`. Git saw main "adding" the block at the old location only because this branch had removed it there, so that copy is dropped rather than duplicated — one `TestSummary` remains. * `TestCaseResultTab.test.tsx` — both sides added a declaration and a reset in the same two places; both are kept. One thing the textual merge could not see: #32982 moved the class base behind a `default` export, so `jest.requireMock('./TestCaseResultTabClassBase')` in the extension-components suite now resolves one level higher than the mock. It is repointed at `.default`, which the tests would otherwise fail on despite a clean merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔄 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 |
🔄 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 |
The banner wrapper carried `tw:pt-4` and nothing below it, so the banner's bottom edge and the tab body grid's top edge measured at the same y — a 0px gap where every other vertical relationship on that grid is 10px (`tw:gap-2.5`). This is not a regression from d2b8875 — `main` has `tw:px-4 tw:pt-4`, so the bottom gap never existed. That commit removed only the horizontal inset, which is what made the omission obvious: once the banner lines up with the cards beneath it, and the reflow puts the result-history card directly under it, the two read as touching. `tw:pb-2.5` matches the grid's own gap, so the banner sits in the same rhythm as the cards rather than being a special case. Measured after: banner bottom 408, grid top 418. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code Review ✅ Approved 3 resolved / 3 findingsReflows the app-mode test case details page to show result history directly beneath the last-run banner with description moved to the right rail, and fixes two pre-existing 16px alignment misalignments in the banner and ✅ 3 resolved✅ Quality: Test leaks isTabExpanded; order-dependent side-panel tests
✅ Quality: Duplicate enableAiAppMode helper diverges from canonical one
✅ Bug: table is never instantiated in beforeAll — all tests crash
OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
🚦 Removed from the merge queue —
|
Describe your changes:
Fixes open-metadata/openmetadata-collate#6416
TCD-0, part of epic open-metadata/openmetadata-collate#6074.
I reflowed the app-mode test case details page so its main column follows the redesign: the result history sits directly under the last-run banner, and the description moves into the right rail alongside the other metadata cards. Previously the chart was the fifth block in the column — you scrolled past Description, Parameter and the SQL section to reach it.
The description's placement was confirmed with product: the redesign does not show it at all, and the side panel was preferred over the bottom of the main column "so it matches the other elements". It keeps its edit affordance, permission gate and change-summary entry; rail order is Description, Tags, Glossary Terms, Data Products, with TCD-10a inserting the Configuration card above them.
While verifying that, two pre-existing 16px misalignments became obvious and are fixed here:
190→1446against the grid's174→1462). Its wrapper carriedtw:px-4while the result-tab grid has no horizontal padding.ObservabilityPageShellgave its content regiontw:p-4but left its header slot unpadded, so every page built on it rendered the header 16px wider than its content (375against391). Visible wherever both are bordered cards — this page and the Incident Manager.Nothing is removed. No API, schema, or data change. Params and the SQL section stay in the main column: the redesign puts them in the right rail, but the rail is 331px and a
tableDifftest'stable2value renders at 440px, so that move waits on the Configuration card's design (TCD-10a).Type of change:
High-level design:
Five files, four of them small. The reflow is a JSX re-order inside
TestCaseResultTab; the two alignment fixes are one Tailwind class each.ObservabilityPageShellis shared, so the header fix shifts the header card inward on five pages: test case details, incident manager, data quality, alerts, test library. I chose padding the header slot over un-padding the content region because it moves one element rather than reflowing every page's content, so no page loses width. Measured all five as flush afterwards. Two further consumers — test suite detail and alert detail — were not reachable with seeded data; test suite detail passes an identically shaped bordered card and benefits the same way. Pipeline observability does not use this shell.data-testid="test-case-rail"is added to the rail container, which had no stable handle; later tickets in the epic mount the Configuration and Test Suites cards into it.Classic mode (
/test-case/:fqn) is intentionally untouched, but it shares the reflowed tab body, so it inherits the main-column order. Its header and tab strip keep today's look.Tests:
Use cases covered
getAdditionalComponentsextension still mounts, below the result historyUnit tests
TestCaseResultTab.test.tsx— main-column order (chart above the parameters), the description rendering inside the rail rather than the main column, and two cases covering the CollategetAdditionalComponentsseam, which was previously mocked to[]and therefore never exercised.components/observability.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts— the shared app-mode details-page spec the rest of the epic extends. Self-contained: builds its own table, test case and result via the API rather than depending on the seeded dataset.y=723below the parameters aty=518; banner alignment reportsx=407against the grid's391; header alignment reports375against391.TestCaseResultPermissions,TestCaseIncidentPermissionsandFailedTestCaseSampleDataall pass — they asserttest-case-result-tab-container,entity-page-header,chart-containerandgraph-containerthrough the reflowed tab.yarn lint:playwright— 0 errors.Manual testing performed
/observability/test-case/…/test-case-resultsfor a Success case (table_column_count_equals) and a Failed case with an incident (diff_columns).ObservabilityPageShellpages.Pre-existing issue found, not fixed here: the test case version page throws
Cannot read properties of undefined (reading '0'). It reproduces onmainwithout this branch, so it is out of scope — worth its own issue.UI screen recording / screenshots:
Before/after screenshots to be attached. The visible changes are the chart moving from fifth in the column to directly beneath the banner, the description moving into the right rail, and the two 16px edges closing.
Checklist:
I have read the CONTRIBUTING document.
My PR is linked to its tracking issue above (Collate repo — see note on the
Fixeskeyword).I have commented on my code, particularly in hard-to-understand areas.
For JSON Schema changes: not applicable, no schema changes.
For UI changes: screenshots to be attached.
I have added tests (unit + Playwright) and listed them above.
I have added tests around the new logic.
🤖 Generated with Claude Code