Refactor(ui): Remove deprecated history typing from withRouteProps

[aryanG9403]

Which problem is this PR solving?

• Part of [🧑‍🎓 LFX]: Upgrading Core Routing and State Management #3313 (Upgrading Core Routing and State Management)

Description of the changes

• Removed deprecated History type usage from withRouteProps
• Simplified IWithRouteProps to rely only on useLocation and useParams
• Removed history prop injection from withRouteProps to reduce coupling with legacy routing APIs
• Aligned the helper with the ongoing React Router v7 migration strategy

How was this change tested?

• Ran yarn lint
• Verified Jaeger UI runs locally without regressions

Checklist

• I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
• I have signed all commits
• I have added unit tests for the new functionality
• I have run lint and test steps successfully
  • for jaeger-ui: yarn lint

[aryanG9403]

Added a follow-up commit to update the withRouteProps test expectations so they match the current React Router behavior. This fixes the failing unit test.

[yurishkuro]

So we're no longer passing history, how is it not a breaking change? THIS file may not need history but we have no idea if the WrappedComponent needs it.

[aryanG9403]

The test update was meant to reflect the current behavior of withRouteProps, which no longer injects history now that routing is handled via hooks.That said, I agree this HOC could still be used by components that expect history, so removing it may be a breaking change depending on usage.
I’m happy to update this in either direction:

1. keep passing history for backward compatibility, or
2. clarify and/or deprecate the behavior explicitly if that’s the intended direction.

Let me know what you’d prefer and I’ll adjust accordingly.

[yurishkuro]

@Parship12

[Parship12]

LGTM 👍

10 components using withRouteProps: 8 of them don't use history and 2 are getting history from other sources. I have tested some and they working properly.

Also the main reason why it does not breaking any functionality is the current routing setup:
from src/index.tsx:

<Router history={history}>
  <CompatRouter>
    <JaegerUIApp />
  </CompatRouter>
</Router>

@yurishkuro could you please review it once more? these changes can sometime be breaking and thy are sometime difficult to catch.

[Parship12]

I found another issue:
there is a buggy code in packages/jaeger-ui/src/components/TracePage/index.tsx

ensureTraceFetched(): Uppercase URLs stay uppercase because the code exits early before normalizing. After fixing the uppercase bug in my local revealed that withRouteProps is not providing history.

[aryanG9403]

Thanks for flagging this 👍
This doesn’t block the current change, so I’ve kept this PR scoped. I’m happy to open a follow-up PR to refactor the TracePage component if we decide to address the normalization bug.

[yurishkuro]

I asked Claude to analyze the existing usage, it came back with two issues:

Summary

8 out of 10 components are completely safe — they don't use history at all. They either:

• Only use location, pathname, search, or params
• Use useNavigate() hook directly for navigation
• Have no navigation logic

2 components currently use history:

1. VirtualizedTraceView.tsx — Uses history in focusSpan() method which calls updateUiFind({ history, location, uiFind })

2. TracePage/index.tsx — Uses history in:

   • clearSearch() via updateUiFind({ history, location, ... })
   • ensureTraceFetched() via history.replace(...)

Problems

The updateUiFind utility already supports both history and navigate, with navigate taking precedence:

  if (navigate) {                                                                                                                     
    navigate({ pathname, search }, { replace: true });                                                                                
  } else if (history) {                                                                                                               
    history.replace({ ...location, search });                                                                                         
  }                                                                                                                                   

However, if history is removed from withRouteProps without updating these two components to pass navigate instead, the updateUiFind calls will effectively become no-ops (neither branch executes), and history.replace() in ensureTraceFetched() will throw a runtime error.

Recommendation

Before merging, verify one of the following:

1. These two components have been updated to use useNavigate() and pass navigate to updateUiFind, OR
2. Add those changes to this PR to prevent runtime errors on the trace page

[aryanG9403]

Thanks for the detailed analysis — that’s very helpful.
You’re right: VirtualizedTraceView and TracePage still rely on history, and removing it from withRouteProps without updating them would lead to no-ops / runtime errors.I’ll update both components in this PR to use useNavigate() and pass navigate to updateUiFind, and replace the remaining history.replace usage accordingly. That should make the change safe and incremental.

Will push an update shortly.

[Parship12]

The updateUiFind utility already supports both history and navigate, with navigate taking precedence

this was done in #3209 for backward compatibility (so that the refactoring can be done properly and incrementally)

history.replace() in ensureTraceFetched() will throw a runtime error.

Yes this is related to the issue the I stated above and in #3477

Recommendation

Before merging, verify one of the following:

1. These two components have been updated to use useNavigate() and pass navigate to updateUiFind, OR

Option 1 will be better, should be done in different PRs

[aryanG9403]

Thanks for the clarification ,
Given the PR-count limits in the contribution guidelines, should the VirtualizedTraceView and TracePage history → navigate refactors be done in a separate follow-up PR by me, or would you prefer to handle those independently?
@yurishkuro

[yurishkuro]

@Parship12 already had a PR for virtualized view #3223. So does TracePage - #3435. You could help by reviewing & testing those PRs.

[aryanG9403]

Got it,
I’ll take a look at #3223 and #3435 and help with review/testing there.

[Parship12]

@aryanG9403 Are you still working on this? Is there any update?

[aryanG9403]

Hey @Parship12!

Currently waiting on:

1. Your new VirtualizedTraceView PR (saw you closed Convert VirtualizedTraceView component from class to functional component #3223
   and mentioned opening a fresh one)
2. PR refactor: convert TracePage from class to functional component #3435 (TracePage migration) to merge first

I have already reviewed and tested PR #3435 locally on Ubuntu
and left my feedback there. Everything looks good from a UI
perspective - no regressions observed.

Once both PRs are in, #3452 should be ready to merge.
Let me know if there's anything I can do to help unblock
things from my side!

[Parship12]

1. Your new VirtualizedTraceView PR (saw you closed Convert VirtualizedTraceView component from class to functional component #3223
   and mentioned opening a fresh one)

could you please continue the refactoring in a new PR, maybe using the review comments from that PR as a reference?

I have already reviewed and tested PR #3435 locally on Ubuntu
and left my feedback there. Everything looks good from a UI
perspective - no regressions observed.

Thanks! could you also help with the code review part?

[aryanG9403]

SURE👍
I’ll take up the VirtualizedTraceView refactor and open a fresh PR based on the previous feedback . I’ll also go through #3435 and review the implementation in more detail.

[aryanG9403]

Hi! @Parship12 ,
I've opened PR #3551 which fully migrates VirtualizedTraceView from a class component to a functional component using useNavigate(), directly addressing the history dependency concern raised here.

However, #3551 is currently on hold due to the contributor PR quota limit. I'll also be doing a proper code review of #3435 shortly.

One question — once #3551 and #3435 are reviewed and approved, what would be the process to get them merged before #3452? Since #3452 depends on both of them landing first.

[Parship12]

One question — once #3551 and #3435 are reviewed and approved, what would be the process to get them merged before #3452? Since #3452 depends on both of them landing first.

we will have to first merge #3551 and #3435 and then we have to do changes here(if require)

[aryanG9403]

Thanks @Parship12 ! ,

could you help by reviewing #3551? It's currently on hold due to the quota limit but all tests are passing and all Copilot review comments have been addressed.

That would help unblock the whole chain!

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[aryanG9403]

Hi @yurishkuro , resolved the merge conflict with main. The PR is now up to date. Could you please re-review when you get a chance?

[Parship12]

Hello @aryanG9403 thanks for your contribution. We have already completed the history removal. I was not able to follow this PR closely since i handled both the history removal and the React router dom v7 upgrade together.