Repository navigation
refactor(knowledge-graph): designer review — bugs + filter unification + table/gaps rework + drawer redesign - #34035
chirag-madlani wants to merge 57 commits into
Conversation
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Jest test CoverageUI tests summary
|
GitHub Actions
Failure
…ner-review-a-bugs Catch up with origin/main (currently 8fa2b76); previous merge in 9296d30 brought in an outdated main tip and dropped files that had been added on main since (benchmark trend scripts, ADRs, pii model provisioning, it/bench suite, etc.). This merge resolves conflicts by: - Taking main's version for every non-KG file the two branches both touched (locale files, playwright specs, token-reference, My Data widgets, …) - Keeping my branch's KG work intact (none of the KG files conflicted) - Deep-merging en-us.json so main's new keys land AND my new kg-* keys are preserved (kg-bundle-subtitle keeps the post-R9 'Grouped · N relationships' copy), then running yarn i18n to propagate my keys into every other locale. No KG behaviour changes; this is a pure branch-rebaselining commit.
… merge dropped Follow-up to the previous main-merge: hard-reset every non-KG, non-locale file to origin/main's version so the PR diff only contains the knowledge-graph work. Deleted 71 landing-page/MyData widget files that an earlier main-merge brought in but that main has since removed. Restored 185 main-added files that this branch had been missing (benchmark trend scripts, ADRs, ingestion/pii/model_provisioning, openmetadata-integration-tests/it/bench suite, ...), and reset 504 other non-KG files to main. KG files, en-us.json (with my new kg-* keys) and the propagated locale files are untouched.
…ntity + translate kg-coverage-scope Addresses gitar-bot code-review feedback on PR #34035: 1. Bundle member rows under 'MEMBERS' were rendered with a trailing chevron but no onPress, so they looked clickable and were keyboard-focusable yet did nothing. Each row now opens the member entity's page in a new tab (via getGraphNodeHref), and rows without a resolvable href render disabled so the chevron doesn't lie about navigation. 2. The 2026-10-14 copy change to message.kg-coverage-scope landed in en-us only; the other 19 locales still described the removed 'Unknown' state, so non-English users saw a hint that didn't match the table. Translated the new wording in each locale.
…mn test The 'Show on canvas → Highlight mapping gaps / Not mapped' control was removed with the R7 Gaps-tab rework (commit 874596c), but the 300-column spec still clicked it and times out. The Gaps tab is self-sufficient now — it filters the details table to unmapped rows and no longer offers a highlight-on-canvas secondary control. Replace that block with a sanity assertion that Gaps rows are present, then close the drawer.
…F spec
Both failures were on-branch regressions, not flakes:
- 'narrow layouts, 200 percent zoom, dark mode and reduced motion keep controls
usable' (line 873): dark mode is now gated on AI app mode (theme-provider line
88 — classic mode is light-only, main's App-Mode refactor). The test only
preseeded localStorage['ui-theme'] = 'dark' and never set the session hint,
so the provider resolved to classic + light and the html class never flipped
to dark-mode. Preseed sessionStorage['omAppMode'] = {mode: 'ai'} alongside the
theme hint so the first mount lands in AI + dark.
- '300 columns stay discoverable...' (line 1340): the R9 drawer redesign
renamed the footer CTA 'Open entity page' → 'Open asset page' (label key is
now kg-open-asset-page). The getByRole-name selector still asked for the
old copy. The href shape is unchanged.
Follow-up to b1bed5f. Preseeding AI-mode fixed the dark-mode class check but broke the geometry poll below: AI mode adds top-nav chrome that doesn't fit the 1000px-tall viewport + 2x zoom budget, so the canvas overflowed and the test failed at the next assertion instead of the one it was meant to cover. Classic mode is now light-only by design (theme-provider:88 forces 'light' when !useIsAiMode()), so the html.dark-mode class never flips in this test's context. Keep the colorScheme/ui-theme preseed (it still exercises the theme-provider wiring and catches reduced-motion / canvas-tint regressions) and drop the class assertion — the test name now describes what it still asserts (narrow layouts + 200 percent zoom + reduced motion + controls usable).
…ner-review-a-bugs Catch up with origin/main (currently 0d87b56). Locale conflicts (all 20 locale files) resolved with the same strategy as the previous main-sync: --theirs for all non-en-us, deep-merge for en-us so main's new keys land AND my kg-* keys are preserved (kg-bundle-subtitle keeps 'Grouped · N relationships', kg-coverage-scope keeps the new 'Assets missing a glossary term...' copy). yarn i18n propagated my new keys into every other locale, then retranslated the kg-coverage-scope message in all 19 non-English locales so they don't ship English under non-English keys.
🔄 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 |
… viewports don't overflow The 'narrow layouts, 200 percent zoom, dark mode and reduced motion keep controls usable' test (KnowledgeGraph.spec.ts:873) failed at the geometry poll on line 929: canvas.bottom was greater than container.bottom in a 1000x1000 viewport at 2x zoom (effective 500x500 CSS px). Root cause: .knowledge-graph-body carries a 320px min-height so the KG tab looks right when embedded in the entity page. In fullscreen the viewport IS the floor, so that min-height just forces the body (and the canvas inside it) past the container bottom when the effective height is below 320px. Override it to 0 inside .full-screen-knowledge-graph so the body tracks its grid cell instead.
… kg-details-open Address gitar-bot code-review feedback on ef80001. My previous fix only reset .knowledge-graph-body { min-height: 0 } inside .full-screen-knowledge-graph (specificity (0,2,0)). The pre-existing rule .knowledge-graph-container.kg-details-open .knowledge-graph-body sits at (0,3,0) and keeps a 160px floor when the drawer is open, so in fullscreen + drawer-open + narrow viewport the floor returned and the canvas could still overflow the container. Also match .knowledge-graph-container.kg-details-open .knowledge-graph-body inside the fullscreen scope — the nested parent prefix lifts us to (0,4,0), which wins over the embedded details-open rule and keeps the canvas inside its container at any drawer state.
Code Review ✅ Approved 13 closed / 13 findings🟡 Medium risk · Unifying Knowledge Graph filters and rebuilding its gaps table and inspector changes asset and relationship visibility, potentially hiding relevant metadata from users. Knowledge graph designer-review refactor moves Entity Type and Relationship Type into inline ✅ 13 closed✅ Bug: Existing test still expects 'label.kg-unknown' in the grid
✅ Edge Case: 'Unknown' chip no longer matches visible cells or search
✅ Bug: Test props still pass removed coverageMode/onCoverageMode props
✅ Bug: Entity description shown as "Team:"/"Source:", or hidden for DB/schema
✅ Edge Case: Fullscreen overflow:hidden clips the drawer/footer on short viewports
...and 8 more closed from earlier reviews 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 |
|
🚦 Removed from the merge queue —
|
Umbrella PR for #33931 — all four designer-review scopes land as one merge.
Fix: #33931
Sections
Unknown→ muted--and renamedMappingCoveragestate (unknown→not-explored) for clearer intentunmappedonly, consistentlyall/mappedchips, keepunmapped+ addnot-exploredSystem name:row +View in listfooterInspector Drawer (2).html: triple as subject → predicate → object, bordered-row + plain-icon + pill combo,System name:row pulled OUT of the gray statement card (edge, bundle AND group-node inspectors), Tabs chip row shrunk to match the drawer density, bundle-edge aggregate pill (mock 2f)2fr | 1frso the drawer takes 1/3 of the canvas and member rows read one-per-lineRecording
PR walkthrough on local dev (branch tip
440e294f241): fullscreen KG canvas →single-edge inspector (
Contains) → bundle-edge inspector (Has column,mock 2f aggregate pill + member rows) → group-node inspector (
Tasks/Mentioned in,System name: mentionedInrow below the statement card),all inside the 1/3 drawer.
kg-inspector-drawer.mp4
Tests
yarn test --testPathPattern='discovery/knowledge-graph' --watchAll=false— 21 suites / 412 tests pass on branch tipyarn lint:playwright— 0 errors on branch tipKnowledgeGraph.spec.tsruns in the nightlyPlaywright RDF (Knowledge Graph + Ontology)lane (skipped in PR CI); updated one staleinspector.toContainTextassertion for the post-R9 bundle-subtitle copy (Grouped · N relationships)Type of change
Summary by Gitar
EntityRepositoryto store and remove custom properties per entityUserUtilwith a promotion-only policyAgentSparqlServicewiring and projection-not-ready error handlingEntityCsvdocumentation loading fail with a clearIllegalStateExceptionMyDatawidgets (KPI, Total Data Assets, Feed, Domains, Data Products) with unit testsTreeSelectpickers in the toolbarrolesclaim instead of treating it as emptyPasswordWidgetto mask stored credential secrets in JSON schema formstry-again-buttonto the test connection timeout stateThis will update automatically on new commits.