Skip to content

chore(UI): upgrade react-router-dom v6 → v7 and fix column panel race condition - #30580

Open
Rohit0301 wants to merge 3 commits into
mainfrom
upgrade-react-router-dom-v7
Open

chore(UI): upgrade react-router-dom v6 → v7 and fix column panel race condition#30580
Rohit0301 wants to merge 3 commits into
mainfrom
upgrade-react-router-dom-v7

Conversation

@Rohit0301

@Rohit0301 Rohit0301 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Describe your changes:

675

I upgraded react-router-dom from 6.30.4 to 7.18.1 and fixed a race condition that the upgrade exposed: in v7, navigate() is wrapped in React.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: bump react-router-dom to 7.18.1
  • useFqnDeepLink.ts (primary fix): move selectedColumn to a useRef so it is read without being a reactive dependency — prevents the hook re-firing when closeColumnDetailPanel sets it to null while columnPart is still stale
  • GenericProvider.tsx (defensive guard): add a skipNextColumnSync ref that is set to true inside closeColumnDetailPanel before navigate(), and consumed once in the URL-sync useEffect, blocking the panel from reopening during the deferred re-render window

SettingsSso.tsx was audited and its existing configFetched ref guard is sufficient — no code change needed there.

Type of change:

  • Improvement

High-level design:

react-router v7 wraps every navigate() / setSearchParams() call in React.startTransition, so the router's React state (useParams, useSearchParams) is updated asynchronously. Local React state (setSelectedColumn(null)) updates synchronously. Any useEffect that 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 skipNextColumnSync flag 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

  • Column detail panel closes and stays closed after clicking the close button
  • Deep-linking to a column FQN in the URL still opens the panel on initial load
  • Navigating between columns (via URL change) still opens the correct panel

Unit tests

  • Not added — the race condition is timing-dependent and best caught by Playwright E2E

Backend integration tests

  • Not applicable (no backend API changes).

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • Existing Table.spec.ts Playwright tests cover the column detail panel close behavior ("Copy column link should have valid URL format" asserts .column-detail-panel is not visible after close)

Manual testing performed

  1. Open a table page with columns
  2. Click a column row to open the column detail panel
  3. Click the close button — verify the panel closes and stays closed
  4. Navigate directly to a URL with a column FQN hash — verify the panel opens correctly

UI screen recording / screenshots:

Not applicable.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
  • For UI changes: I attached a screen recording and/or screenshots above.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.

Greptile Summary

The PR upgrades the UI router and adjusts column-detail synchronization to accommodate deferred navigation.

  • Pins react-router-dom at 7.18.1 and updates its transitive lockfile entries.
  • Reads selected-column state through refs to avoid reactive effects reopening a recently closed detail panel.
  • Stabilizes the column-opening callback by removing selected-column state from its dependency list.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains within the scope of the eligible follow-up findings.

Important Files Changed

Filename Overview
openmetadata-ui/src/main/resources/ui/package.json Pins react-router-dom to version 7.18.1.
openmetadata-ui/src/main/resources/ui/yarn.lock Updates the resolved React Router dependency graph and associated transitive packages.
openmetadata-ui/src/main/resources/ui/src/components/Customization/GenericProvider/GenericProvider.tsx Stabilizes column-panel opening logic by reading the selected column through a ref.
openmetadata-ui/src/main/resources/ui/src/hooks/useFqnDeepLink.ts Prevents selected-column state changes from independently retriggering deep-link synchronization.

Sequence Diagram

sequenceDiagram
  participant User
  participant Panel as Column detail panel
  participant State as Selected-column state/ref
  participant Router
  participant DeepLink as Deep-link effect
  User->>Panel: Close panel
  Panel->>State: Set selected column to null
  Panel->>Router: Navigate without column FQN
  Router-->>DeepLink: Deferred route update
  DeepLink->>State: Read current selected-column ref
  DeepLink-->>Panel: Synchronize panel with updated route
Loading

Reviews (3): Last reviewed commit: "Merge branch 'main' into upgrade-react-r..." | Re-trigger Greptile

… 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>
@Rohit0301
Rohit0301 requested a review from a team as a code owner July 28, 2026 11:59
@github-actions

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

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 skip-pr-checks label.

@github-actions github-actions Bot added the UI UI specific issues label Jul 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

🔴 Playwright Results — workflow failed

Validated commit d006158e52d3b94432fcd2faa31b99fbd4cc03e8 in Playwright run 30383070890, attempt 1.

✅ 426 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 5 skipped · 🧰 0 lifecycle flaky

Pipeline and setup failures (3)

  • Playwright coverage validation found 110 missing, 0 unexpected, 0 duplicate-plan, and 0 duplicate-execution test ID(s).
  • Shard chromium-02 did not upload a usable Playwright results artifact.
  • Shard chromium-02 test execution finished with status failure without a reported test failure.

Performance

Blocking targets: ✅ met · 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) 58m 36s

⏱️ Max setup 2m 51s · max shard execution 21m 0s · max shard-job elapsed before upload 24m 21s · reporting 5s

🌐 254.62 requests/attempt · 2.86 app boots/UI scenario · 43.25% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 43.25% (convergence target: at most 15%).
  • Browser traffic was 254.62 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.86 per UI scenario (1600 boots / 560 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 86 0 0 0 0 0
⛔ Shard chromium-02
✅ Shard chromium-03 100 0 0 0 0 0
✅ Shard chromium-04 103 0 0 3 0 0
✅ Shard data-asset-rules-01 61 0 0 0 0 0
✅ Shard domain-isolation-01 14 0 0 0 0 0
✅ Shard global-state-01 23 0 0 0 0 0
✅ Shard ingestion-01 1 0 0 0 0 0
✅ Shard reindex-01 2 0 0 0 0 0
✅ Shard search-01 10 0 0 0 0 0
🟡 Shard search-rbac-01 26 0 1 2 0 0
🟡 1 flaky test(s) (passed on retry)
  • Flow/SearchRBAC.spec.tsa user permitted on all asset types browses both (shard search-rbac-01, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

Comment thread openmetadata-ui/src/main/resources/ui/src/hooks/useFqnDeepLink.ts
@Rohit0301 Rohit0301 self-assigned this Jul 28, 2026
@Rohit0301 Rohit0301 added the safe to test Add this label to run secure Github workflows on PRs label Jul 28, 2026
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.79% (76990/117012) 49.64% (46259/93175) 50.89% (13932/27375)

@gitar-bot

gitar-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 2 resolved / 2 findings

Upgrades React Router DOM to v7 and resolves column detail panel race conditions using non-reactive refs and a one-shot synchronization guard. No issues found.

✅ 2 resolved
Bug: useFqnDeepLink still re-fires on close via openColumnDetailPanel dep

📄 openmetadata-ui/src/main/resources/ui/src/hooks/useFqnDeepLink.ts:35-49 📄 openmetadata-ui/src/main/resources/ui/src/components/Customization/GenericProvider/GenericProvider.tsx:261-263 📄 openmetadata-ui/src/main/resources/ui/src/components/Customization/GenericProvider/GenericProvider.tsx:426
The stated primary fix (moving selectedColumn to a ref) does not actually stop the reopen race. The effect's dependency array still contains openColumnDetailPanel, and that callback comes from useGenericContext() where it is memoized with selectedColumn?.fullyQualifiedName in its own dep array (GenericProvider.tsx:233-263). So when closeColumnDetailPanel synchronously runs setSelectedColumn(null), openColumnDetailPanel gets a new identity, which re-triggers the useFqnDeepLink effect. Because navigate() is wrapped in React.startTransition in react-router v7, fqn/columnPart (derived from useParams) are still stale (the old column), so findFieldByFQN returns the just-closed column and — since selectedColumnRef.current is now null — the guard null?.fqn !== matched.fqn is true and openColumnDetailPanel(matchedField) reopens the panel. openColumnDetailPanel no longer short-circuits because selectedColumn is null. Fix: also read openColumnDetailPanel (and setExpandedRowKeys) through refs and drop them from the dependency array so the effect only reacts to real URL (fqn/columnPart) and data changes; that way the effect only runs when the URL truly updates (to empty), yielding an early return.

Edge Case: skipNextColumnSync one-shot flag can be consumed by unrelated re-run

📄 openmetadata-ui/src/main/resources/ui/src/components/Customization/GenericProvider/GenericProvider.tsx:140-152 📄 openmetadata-ui/src/main/resources/ui/src/components/Customization/GenericProvider/GenericProvider.tsx:265-279
The skipNextColumnSync guard blocks the very next execution of the sync effect regardless of what triggered it. The effect depends on [extractedColumns, columnFqn, cleanColumn], so if extractedColumns changes (e.g. a background data refresh) during the deferred-navigate window before columnFqn updates, that unrelated run consumes the flag, leaving the actual stale-columnFqn run unguarded. Conversely, if closeColumnDetailPanel runs but columnFqn never changes as a result, the flag stays true and will wrongly suppress the next legitimate deep-link sync. Consider keying the skip to the specific fqn being closed (store the closed columnFqn in the ref and only skip when the effect still sees that same stale value) instead of a blanket one-shot boolean.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant