chore(UI): upgrade react-router-dom v6 → v7 and fix column panel race condition - #30580
chore(UI): upgrade react-router-dom v6 → v7 and fix column panel race condition#30580Rohit0301 wants to merge 21 commits into
Conversation
… condition - Bump react-router-dom from 6.30.4 to 7.18.1 - In v7, navigate() is wrapped in React.startTransition, deferring the router re-render. This caused the column detail panel to reopen after close because effects still saw stale URL params during the transition. - Fix useFqnDeepLink.ts: move selectedColumn to a ref so it is read without triggering the effect when closeColumnDetailPanel sets it null. - Fix GenericProvider.tsx: add skipNextColumnSync ref that is set before navigate() in closeColumnDetailPanel and consumed once in the URL-sync useEffect, preventing the panel from reopening during the deferred re-render window. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
❌ 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 |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
🔴 Playwright Results — workflow failedValidated commit ✅ 4097 passed · ❌ 2 failed · 🟡 36 flaky · ⏭️ 2 skipped · 🧰 0 lifecycle flaky Pipeline and setup failures (6)
PerformanceBlocking targets: ❌ unmet · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 59m 23s ⏱️ Max setup 3m 11s · max shard execution 25m 0s · max shard-job elapsed before upload 28m 48s · reporting 42s 🌐 220.28 requests/attempt · 2.30 app boots/UI scenario · 49.20% common-shard skew Optimization targets still in progress:
Genuine Failures (failed on all attempts)❌
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Rohit0301
added
the
safe to test
Jest test CoverageUI tests summary
|
|
| await customTab.focus(); | ||
| await userPage.keyboard.press('Enter'); | ||
| await waitForAllLoadersToDisappear(userPage); | ||
| await userPage.waitForLoadState('domcontentloaded'); | ||
|
|
||
| await waitForAllLoadersToDisappear( | ||
| userPage, | ||
| 'entity-detail-widget-skeleton' | ||
| ); |
There was a problem hiding this comment.
💡 Edge Case: Re-adds no-op waitForLoadState after SPA tab switch
This commit removes the clickTabAndWaitForPanel helper — which waited for the target tabpanel to become active — and reverts to focus() + Enter followed by waitForLoadState('domcontentloaded'). As the just-deleted comments documented, domcontentloaded is a no-op on an SPA tab switch (no real navigation occurs) and provides no synchronization against react-router v7's deferred navigate(), so the previous tab's content can still be in the DOM when the following assertions run. Flakiness is partly mitigated here because each test subsequently waits for a tab-specific element (KnowledgePanel.Description-* / description widget), but consider waiting for the active tabpanel to reintroduce a real guard instead of the no-op load-state wait.
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 6 resolved / 7 findingsUpgrades react-router-dom from v6 to v7 and resolves column panel race conditions using refs and a skip-sync flag. Consider addressing the minor waitForLoadState usage in SPA tab switches. 💡 Edge Case: Re-adds no-op waitForLoadState after SPA tab switch📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/CustomizeDetailPage.spec.ts:519-527 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/CustomizeDetailPage.spec.ts:692-699 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DescriptionVisibility.spec.ts:420-424 This commit removes the ✅ 6 resolved✅ Bug: useFqnDeepLink still re-fires on close via openColumnDetailPanel dep
✅ Edge Case: skipNextColumnSync one-shot flag can be consumed by unrelated re-run
✅ Bug: Duplicate const declaration of selectedColumnRef breaks build
✅ Quality: Redundant useEffect re-assigns selectedColumnRef already set synchronously
✅ Quality: Stale comment references rAF frames not present in code
...and 1 more resolved from earlier reviews 🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
❌ Playwright - ESLint + Prettier + Organise ImportsOne or more Playwright test files have linting or formatting issues. Affected files
🔍 ESLint findings in this PR's files — 0 error(s), 57 warning(s)Errors block the build. Warnings do not yet — they are rules whose backlog is still 0 error(s), 57 warning(s) across 11 changed file(s).
All findings
… and 7 more. Run Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
1 participant
Describe your changes:
675
I upgraded
react-router-domfrom6.30.4to7.18.1and fixed a race condition that the upgrade exposed: in v7,navigate()is wrapped inReact.startTransition, which defers the router re-render. This caused the column detail panel to briefly reopen after closing because effects still saw stale URL params during the transition window.package.json/yarn.lock: bumpreact-router-domto7.18.1useFqnDeepLink.ts(primary fix): moveselectedColumnto auseRefso it is read without being a reactive dependency — prevents the hook re-firing whencloseColumnDetailPanelsets it tonullwhilecolumnPartis still staleGenericProvider.tsx(defensive guard): add askipNextColumnSyncref that is set totrueinsidecloseColumnDetailPanelbeforenavigate(), and consumed once in the URL-syncuseEffect, blocking the panel from reopening during the deferred re-render windowSettingsSso.tsxwas audited and its existingconfigFetchedref guard is sufficient — no code change needed there.Type of change:
High-level design:
react-router v7 wraps every
navigate()/setSearchParams()call inReact.startTransition, so the router's React state (useParams,useSearchParams) is updated asynchronously. Local React state (setSelectedColumn(null)) updates synchronously. AnyuseEffectthat depends on both will fire immediately with a mix of new local state and stale URL params — creating a reopen race.The two fixes use the "ref as a non-reactive value" pattern: store the value in a ref so it can be read inside an effect without being a trigger for it. The
skipNextColumnSyncflag is a one-shot guard: set before navigate, consumed once on the next effect run, then reset — so deep-link opens on initial mount and subsequent URL-driven opens are unaffected.Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Table.spec.tsPlaywright tests cover the column detail panel close behavior ("Copy column link should have valid URL format"asserts.column-detail-panelis not visible after close)Manual testing performed
UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Greptile Summary
The PR upgrades React Router DOM from 6.30.4 to 7.18.1 and adjusts column-detail synchronization to avoid reopening the panel while navigation is deferred.
react-router-domat 7.18.1 and refreshes its transitive lockfile entries.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
Reviews (5): Last reviewed commit: "Merge branch 'main' into upgrade-react-r..." | Re-trigger Greptile
Summary by Gitar
processedPageSizeRefinusePagingto prevent URL sync effects from resetting page size on stale statecommon.ts,TestLibrary.spec.ts,ImpactAnalysis.spec.ts, etc.) to handle React Router v7 navigation timingThis will update automatically on new commits.