fix: Normalize uppercase trace ids to lowercase in URL#3485
fix: Normalize uppercase trace ids to lowercase in URL#3485yurishkuro merged 12 commits intojaegertracing:mainfrom
Conversation
|
Hi @samar-703, thanks for your contribution! To ensure quality reviews, we limit how many concurrent open PRs new contributors can open. This PR is currently on hold (Status: 2/1 open). We will automatically move this into the review queue once your existing PRs are merged or closed. Please see our Contributing Guidelines for details on our tiered quota policy. |
There was a problem hiding this comment.
Pull request overview
This PR refactors how trace ID URL normalization is handled for the Trace page so that uppercase trace IDs are normalized to lowercase at the routing layer, aiming to fix #3477 and to preserve query parameters.
Changes:
- Moves trace ID normalization out of
TracePageImpl.ensureTraceFetchedinto auseEffecthook in the functionalTracePagewrapper that usesuseNavigatefromreact-router-dom-v5-compat. - Uses
useLocationto preserve the existing query string (location.search) during normalization redirects. - Updates the
"forces lowercase id"test inindex.test.jsto stop asserting onhistory.replaceand instead perform a minimal render assertion with an uppercase ID.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/jaeger-ui/src/components/TracePage/index.tsx | Adds a hook-based URL-normalization step in the functional TracePage wrapper and removes the old history.replace(getLocation(...)) logic from TracePageImpl.ensureTraceFetched. |
| packages/jaeger-ui/src/components/TracePage/index.test.js | Adjusts the "forces lowercase id" test to no longer rely on history.replace and to render TracePageImpl with an uppercase ID, but without asserting on the new navigation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Parship12
left a comment
There was a problem hiding this comment.
LGTM 👍
we could do this with a simpler fix (by reordering logic in ensureTraceFetched), but since we are planning to convert this component to functional component with hooks, this approach makes sense as it moves us in that direction. I think we may need to reorganize the logic, but we can handle that during the full migration.
Screen.Recording.2026-01-31.194943.mp4 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yurishkuro
left a comment
There was a problem hiding this comment.
Please respond to copilot comments
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I've made the required changes to address the Copilot feedback:
The solution is using the existing getUrl() utility function, which properly handles deployment URL prefixes via prefixUrl(). tested locally - uppercase trace IDs now normalize to lowercase without redirecting. Note: Copilot is now suggesting adding tests for the wrapper component's normalization behavior. I can add those if needed, though it would require setting up proper mocking for the default export with router context. Let me know if you'd like me to add wrapper tests or if the current test coverage is sufficient for this fix. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/jaeger-ui/src/components/TracePage/index.integration.test.js
Outdated
Show resolved
Hide resolved
662a70f to
606a720
Compare
606a720 to
0b2240d
Compare
…ce ID normalization Signed-off-by: Samar <hello.samar7@gmail.com>
- normalize trace ID to lowercase before passing to component - preserve query parameters and location state during redirect - added integration tests for normalization and query param preservation. Signed-off-by: Samar <hello.samar7@gmail.com>
Signed-off-by: Samar <hello.samar7@gmail.com>
- Extract uppercase-to-lowercase normalization into useNormalizeTraceId hook - Hook handles redirect and preserves query params and location state - Replace integration test with focused unit tests for the hook - Simplifies TracePage component by removing inline navigation logic Fixes double-fetch caused by uppercase trace IDs in Redux store. Signed-off-by: Samar <hello.samar7@gmail.com>
Signed-off-by: Samar <hello.samar7@gmail.com>
4d28534 to
9097997
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3485 +/- ##
==========================================
+ Coverage 89.04% 89.15% +0.10%
==========================================
Files 302 304 +2
Lines 9613 9719 +106
Branches 2547 2587 +40
==========================================
+ Hits 8560 8665 +105
- Misses 1050 1051 +1
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const TracePage = (props: TracePageProps) => { | ||
| const config = useConfig(); | ||
| const traceID = props.params.id; | ||
| const normalizedTraceID = useNormalizeTraceId(traceID); | ||
|
|
||
| return ( | ||
| <ConnectedTracePage | ||
| {...props} | ||
| params={{ ...props.params, id: normalizedTraceID }} | ||
| archiveEnabled={Boolean(config.archiveEnabled)} |
There was a problem hiding this comment.
The URL-normalization behavior is implemented in the functional TracePage wrapper (hook call + passing a normalized params.id), but the existing index.test.js suite only renders TracePageImpl directly. The new unit tests cover useNormalizeTraceId(), but they don’t verify that the real TracePage route actually invokes the hook and performs navigation when params.id is uppercase. Adding a small integration test that renders the default export (withRouteProps wrapper) under a router and asserts useNavigate(..., { replace: true }) is called would prevent regressions where the hook is accidentally removed or the normalized params.id is no longer passed down.
|
@samar-703 Update the copyright year to 2026 in the files to fix the CI issue. Also, is there a way to fix the drop in code coverage? |
jkowall
left a comment
There was a problem hiding this comment.
Review
Summary
This PR extracts trace ID case normalization from the TracePageImpl class component's ensureTraceFetched method into a new useNormalizeTraceId custom hook used by the functional TracePage wrapper. This fixes #3477 where uppercase trace IDs in URLs caused issues.
+98 / -17 across 4 files (2 new, 2 modified)
What's Good
- Clean separation of concerns: Extracting normalization into a dedicated hook is the right pattern, especially since the project is migrating toward functional components.
- Query params preserved: The hook correctly appends
location.searchto the redirect URL. - Replace navigation: Using
{ replace: true }avoids polluting browser history. - Tests are well-structured: The hook tests cover the three key scenarios (uppercase -> redirect, query param preservation, already-lowercase -> no-op).
Issues & Suggestions
1. Bug: location in useEffect dependency causes infinite loop risk
useNormalizeTraceId.ts:20 — location.state is in the dependency array but location is a new object on every render in React Router. While location.search is a primitive string (safe), location.state is an object reference that can change identity without changing value, potentially causing unnecessary re-runs.
Suggestion: Remove location.state from the dependency array, or capture it with a ref. The redirect only needs to fire when traceID changes.
React.useEffect(() => {
if (traceID && traceID !== normalizedTraceID) {
const url = getUrl(normalizedTraceID);
navigate(`${url}${location.search}`, { replace: true, state: location.state });
}
}, [traceID, normalizedTraceID]); // navigate and location accessed via closure2. Bug: getUrl mock in tests doesn't match real signature
useNormalizeTraceId.test.ts:15-17 — The mock getUrl: (id: string) => /trace/${id} ignores the optional uiFind parameter and more importantly, doesn't use prefixUrl(). This means tests won't catch issues with URL prefix handling. Consider using the real getUrl or at least documenting why the simplification is intentional.
3. Potential double-fetch still exists
The PR description mentions fixing "double-fetch caused by uppercase trace IDs in Redux store," but look at the flow:
TracePagerenders with uppercasetraceIDuseNormalizeTraceIdreturns the lowercase ID immediately (before the effect fires)ConnectedTracePagereceives the lowercase ID and passes it toTracePageImplensureTraceFetchedcallsfetchTrace(id)with the lowercase ID- The
useEffectfires and triggers a navigation, which re-renders
This means fetchTrace is called with the correct lowercase ID on first render, but the navigation in step 5 causes a second render cycle. If React Router re-mounts the component on navigation, fetchTrace could be called again. Worth verifying this doesn't cause a double fetch.
4. ensureTraceFetched lost the early return
index.tsx:292-296 — The original code had:
if (!trace) {
fetchTrace(id);
return; // <-- this early return was removed
}The return was there to skip the normalization check below it. Now that the normalization check is gone, removing return is fine functionally. But note that the location destructuring was also removed from ensureTraceFetched, while location is still in the props type — just confirming that's intentional and location isn't used elsewhere in this method.
5. Test weakened for TracePageImpl
index.test.js:348-361 — The old test verified that history.replace was called with the correct lowercase path. The new test only checks expect(() => render(...)).not.toThrow(), which doesn't actually verify any behavior. Since normalization now lives in the wrapper, this is somewhat acceptable, but the test name "forces lowercase id" is now misleading — it should be renamed to something like "renders without error when given uppercase id".
6. Minor: getLocation is now unused
The diff removes the import of getLocation from index.tsx, but getLocation is still exported from url/index.ts. Verify no other consumers use it; if not, it should be removed to avoid dead code.
7. Copyright year
useNormalizeTraceId.ts:1 and useNormalizeTraceId.test.ts:1 — Both files have Copyright (c) 2025 but the current year is 2026.
Verdict
The overall approach is sound and aligns with the project's direction toward hooks. The main concern is the location.state in the useEffect dependency array which could cause re-render loops in certain scenarios. The test coverage for the hook is good but the existing TracePageImpl test was weakened without a meaningful replacement assertion.
jkowall
left a comment
There was a problem hiding this comment.
Review
Summary
This PR extracts trace ID case normalization from the TracePageImpl class component's ensureTraceFetched method into a new useNormalizeTraceId custom hook used by the functional TracePage wrapper. This fixes #3477 where uppercase trace IDs in URLs caused issues.
+98 / -17 across 4 files (2 new, 2 modified)
What's Good
- Clean separation of concerns: Extracting normalization into a dedicated hook is the right pattern, especially since the project is migrating toward functional components.
- Query params preserved: The hook correctly appends
location.searchto the redirect URL. - Replace navigation: Using
{ replace: true }avoids polluting browser history. - Tests are well-structured: The hook tests cover the three key scenarios (uppercase -> redirect, query param preservation, already-lowercase -> no-op).
Issues & Suggestions
1. Bug: location in useEffect dependency causes infinite loop risk
useNormalizeTraceId.ts:20 — location.state is in the dependency array but location is a new object on every render in React Router. While location.search is a primitive string (safe), location.state is an object reference that can change identity without changing value, potentially causing unnecessary re-runs.
Suggestion: Remove location.state from the dependency array, or capture it with a ref. The redirect only needs to fire when traceID changes.
React.useEffect(() => {
if (traceID && traceID !== normalizedTraceID) {
const url = getUrl(normalizedTraceID);
navigate(`${url}${location.search}`, { replace: true, state: location.state });
}
}, [traceID, normalizedTraceID]); // navigate and location accessed via closure2. Bug: getUrl mock in tests doesn't match real signature
useNormalizeTraceId.test.ts:15-17 — The mock getUrl: (id: string) => /trace/${id} ignores the optional uiFind parameter and more importantly, doesn't use prefixUrl(). This means tests won't catch issues with URL prefix handling. Consider using the real getUrl or at least documenting why the simplification is intentional.
3. Potential double-fetch still exists
The PR description mentions fixing "double-fetch caused by uppercase trace IDs in Redux store," but look at the flow:
TracePagerenders with uppercasetraceIDuseNormalizeTraceIdreturns the lowercase ID immediately (before the effect fires)ConnectedTracePagereceives the lowercase ID and passes it toTracePageImplensureTraceFetchedcallsfetchTrace(id)with the lowercase ID- The
useEffectfires and triggers a navigation, which re-renders
This means fetchTrace is called with the correct lowercase ID on first render, but the navigation in step 5 causes a second render cycle. If React Router re-mounts the component on navigation, fetchTrace could be called again. Worth verifying this doesn't cause a double fetch.
4. ensureTraceFetched lost the early return
index.tsx:292-296 — The original code had:
if (!trace) {
fetchTrace(id);
return; // <-- this early return was removed
}The return was there to skip the normalization check below it. Now that the normalization check is gone, removing return is fine functionally. But note that the location destructuring was also removed from ensureTraceFetched, while location is still in the props type — just confirming that's intentional and location isn't used elsewhere in this method.
5. Test weakened for TracePageImpl
index.test.js:348-361 — The old test verified that history.replace was called with the correct lowercase path. The new test only checks expect(() => render(...)).not.toThrow(), which doesn't actually verify any behavior. Since normalization now lives in the wrapper, this is somewhat acceptable, but the test name "forces lowercase id" is now misleading — it should be renamed to something like "renders without error when given uppercase id".
6. Minor: getLocation is now unused
The diff removes the import of getLocation from index.tsx, but getLocation is still exported from url/index.ts. Verify no other consumers use it; if not, it should be removed to avoid dead code.
7. Copyright year
useNormalizeTraceId.ts:1 and useNormalizeTraceId.test.ts:1 — Both files have Copyright (c) 2025 but the current year is 2026.
Verdict
The overall approach is sound and aligns with the project's direction toward hooks. The main concern is the location.state in the useEffect dependency array which could cause re-render loops in certain scenarios. The test coverage for the hook is good but the existing TracePageImpl test was weakened without a meaningful replacement assertion.
|
PR quota unlocked! @samar-703, this PR has been moved out of the waiting room and into the active review queue:
Thank you for your patience. |
- Fix copyright year to 2026 in new files - Remove location.state from useEffect deps to prevent re-render loops; navigate and location accessed via closure instead - Rename misleading test 'forces lowercase id' to accurately reflect that normalization now lives in the wrapper hook - Document simplified getUrl mock intent in hook unit tests Signed-off-by: Samar <hello.samar7@gmail.com>
done! updated copyright year to 2026 in both new files and added a DOM |
jkowall
left a comment
There was a problem hiding this comment.
Looks good thanks for the contribution
Which problem is this PR solving?
Description of the changes
How was this change tested?
http://localhost:5173/trace/B36DE06C5972AB071AC119C504CA07DCsuccessfully auto-normalizes to lowercaseChecklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test