chore: Remove unsafe use of _get()#3606
Open
IstvanCsVarga wants to merge 1 commit intojaegertracing:mainfrom
Open
chore: Remove unsafe use of _get()#3606IstvanCsVarga wants to merge 1 commit intojaegertracing:mainfrom
IstvanCsVarga wants to merge 1 commit intojaegertracing:mainfrom
Conversation
yurishkuro
reviewed
Mar 13, 2026
| let details = _get(res, getDetailPath as string); | ||
| let details: TDetails | undefined = (getDetailPath as string) | ||
| .split('.') | ||
| .reduce<unknown>((obj, key) => (obj as Record<string, unknown>)?.[key], res) as |
Author
There was a problem hiding this comment.
Fair point, for these 3 files with dynamic paths (where the path string comes from config at runtime), the inline replacement is more verbose. I see a few options:
- Keep
_getin just these 3 files where paths are truly dynamic - Extract a small
getByPathutility (though that's basically reimplementing_get)
Member
There was a problem hiding this comment.
I would keep _get, but I would also question why is it that the paths are dynamic, it feels fishy. When there are truly dynamic values the data structure would be a map, not an object.
Author
There was a problem hiding this comment.
Agreed, I'll revert the dynamic-path files to keep _get. The dynamic paths do come from the user-configured decoration schemas (summaryPath, detailPath, etc.) that navigate arbitrary API response structures, so the paths are actually runtime-dependent. But restructuring that is probably a another discussion
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes remaining usages of lodash/get (_get()) across Jaeger UI and replaces them with native optional chaining / nullish coalescing (and explicit dot-path traversal where paths are dynamic), improving type-safety and making missing properties compiler-visible.
Changes:
- Replaced
_get()calls with?./??across UI, reducers, and utilities. - Implemented explicit runtime dot-path traversal (
split('.').reduce(...)) for dynamically-computed paths. - Updated TypeScript types to reflect previously-implicit state/config shape (
ReduxState.pathAgnosticDecorations,Config.tracking.cookiesToDimensions).
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/jaeger-ui/src/utils/tracking/ga.ts | Uses optional chaining for tracking config access and window.location.search parsing. |
| packages/jaeger-ui/src/utils/plexus/set-on-graph.ts | Replaces _get() defaulting with ?? for zoom scale access. |
| packages/jaeger-ui/src/utils/config/process-deprecation.ts | Replaces _get() with dot-path traversal when transferring deprecated config values. |
| packages/jaeger-ui/src/utils/DraggableManager/DraggableManager.ts | Replaces _get(document, 'body.style') with document.body?.style. |
| packages/jaeger-ui/src/types/index.ts | Adds pathAgnosticDecorations to ReduxState type. |
| packages/jaeger-ui/src/types/config.ts | Adds tracking.cookiesToDimensions to config typing. |
| packages/jaeger-ui/src/reducers/embedded.ts | Replaces _get(window, 'location.search') with window.location?.search. |
| packages/jaeger-ui/src/model/path-agnostic-decorations/index.tsx | Uses typed optional chaining for decoration state extraction. |
| packages/jaeger-ui/src/components/TracePage/index.tsx | Replaces _get() access for vertices/spans in UI find logic. |
| packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanTreeOffset.tsx | Replaces _get() access to relatedTarget.dataset with optional chaining. |
| packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageHeader.tsx | Replaces _get() defaulting for computed “depth” header value. |
| packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/traceDiffGraphUtils.ts | Replaces _get(...,'size') with optional chaining on size. |
| packages/jaeger-ui/src/components/SearchTracePage/SearchForm.tsx | Replaces _get() state reads with optional chaining on state.config.search. |
| packages/jaeger-ui/src/components/DeepDependencies/traces.tsx | Replaces _get() for graphState.model.hash with safe discriminated access. |
| packages/jaeger-ui/src/components/DeepDependencies/index.tsx | Replaces _get() usage for hash and DDG state entry computed key access. |
| packages/jaeger-ui/src/components/DeepDependencies/SidePanel/DetailsPanel.tsx | Replaces _get() with dot-path traversal for decoration detail/column defs extraction. |
| packages/jaeger-ui/src/actions/path-agnostic-decorations.ts | Replaces _get() with dot-path traversal + nullish defaulting for decoration values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
IstvanCsVarga
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses unsafe lodash/get (_get) usage by replacing most calls with native optional chaining (?.) and nullish coalescing (??) to make property access more type-safe and compiler-checked across Jaeger UI.
Changes:
- Replaced many
_get()usages with?./??in UI, reducers, and utilities. - Extended type definitions to reflect actual state/config fields used at runtime (e.g., Redux state slice and tracking config).
- Kept dynamic runtime-dependent access patterns in the few places where paths are genuinely dynamic (per PR description).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/jaeger-ui/src/utils/tracking/ga.ts | Removes _get usage for GA config/query access; uses optional chaining on tracking config. |
| packages/jaeger-ui/src/utils/plexus/set-on-graph.ts | Replaces _get default access for zoom scale with ?. / ??. |
| packages/jaeger-ui/src/utils/DraggableManager/DraggableManager.ts | Replaces _get(document, 'body.style') with document.body?.style. |
| packages/jaeger-ui/src/types/index.ts | Adds pathAgnosticDecorations to ReduxState typing. |
| packages/jaeger-ui/src/types/config.ts | Adds tracking.cookiesToDimensions typing. |
| packages/jaeger-ui/src/reducers/embedded.ts | Replaces _get(window, 'location.search') with window.location?.search. |
| packages/jaeger-ui/src/model/path-agnostic-decorations/index.tsx | Replaces dynamic _get paths with structured optional chaining/bracket access. |
| packages/jaeger-ui/src/components/TracePage/index.tsx | Replaces _get usage for vertices/spans with optional chaining/defaults. |
| packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanTreeOffset.tsx | Replaces _get on relatedTarget dataset access with direct access + assertions. |
| packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageHeader.tsx | Replaces _get around _maxBy() result with ?. / ??. |
| packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/traceDiffGraphUtils.ts | Replaces _get(..., 'size') with ?.size. |
| packages/jaeger-ui/src/components/SearchTracePage/SearchForm.tsx | Replaces _get(state, 'config.search.*') with optional chaining. |
| packages/jaeger-ui/src/components/DeepDependencies/traces.tsx | Replaces _get(graphState, 'model.hash') with guarded direct access. |
| packages/jaeger-ui/src/components/DeepDependencies/index.tsx | Replaces _get for hash/state access and uses optional chaining for keyed lookups. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Removes most uses of lodash/get in jaeger-ui in favor of optional chaining / nullish coalescing to make property access type-safe and compiler-checked (per #3604), plus updates a couple of types that _get() had been masking.
Changes:
- Replaced
_get()calls across UI/redux/utils code with?./??(keeping_getonly where paths are runtime-dynamic). - Added missing type fields:
ReduxState.pathAgnosticDecorationsandConfig.tracking.cookiesToDimensions. - Adjusted several call sites to preserve previous defaults (e.g.
?? [],?? 1) and improve event target handling.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/jaeger-ui/src/utils/tracking/ga.ts | Replaces _get(config, ...) and _get(window, ...) with optional chaining; relies on updated tracking config typing. |
| packages/jaeger-ui/src/utils/plexus/set-on-graph.ts | Replaces _get(zoomTransform, 'k', 1) with zoomTransform?.k ?? 1 for default scaling. |
| packages/jaeger-ui/src/utils/DraggableManager/DraggableManager.ts | Replaces _get(document, 'body.style') with document.body?.style. |
| packages/jaeger-ui/src/types/index.ts | Adds pathAgnosticDecorations slice to ReduxState. |
| packages/jaeger-ui/src/types/config.ts | Adds tracking.cookiesToDimensions to the Config type. |
| packages/jaeger-ui/src/reducers/embedded.ts | Replaces _get(window, 'location.search') with window.location?.search. |
| packages/jaeger-ui/src/model/path-agnostic-decorations/index.tsx | Replaces dynamic _get(state, \pathAgnosticDecorations...`)` with direct typed access into the decorations state. |
| packages/jaeger-ui/src/components/TracePage/index.tsx | Replaces _get() usages for trace DAG vertices/spans with ?./?? equivalents. |
| packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanTreeOffset.tsx | Replaces _get(event, 'relatedTarget.dataset...') with a local relatedTarget variable and direct access after type guard. |
| packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageHeader.tsx | Replaces _get(_maxBy(...), 'depth', 0) with ?.depth ?? 0. |
| packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/traceDiffGraphUtils.ts | Replaces _get(set, 'size') with set?.size. |
| packages/jaeger-ui/src/components/SearchTracePage/SearchForm.tsx | Replaces _get(state, 'config.search.*') with state.config?.search?.*. |
| packages/jaeger-ui/src/components/DeepDependencies/traces.tsx | Replaces _get(graphState, 'model.hash') with a structural guard and direct access. |
| packages/jaeger-ui/src/components/DeepDependencies/index.tsx | Replaces _get() with optional chaining / bracket access for ddg state entry + hash extraction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yurishkuro
reviewed
Mar 30, 2026
| const getUrlArg = { uiFind, ...urlState, ...newValues, ...extraUrlArgs }; | ||
| const hash = _get(graphState, 'model.hash'); | ||
| const hash = | ||
| graphState && graphState.state === fetchedState.DONE |
Member
There was a problem hiding this comment.
Suggested change
| graphState && graphState.state === fetchedState.DONE | |
| graphState?.state === fetchedState.DONE |
Author
There was a problem hiding this comment.
Applied in 4f5c4e4. Also applied the same simplification to the identical pattern in mapStateToProps for consistency.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes most remaining usages of lodash/get (_get) in jaeger-ui in favor of optional chaining (?.) and nullish coalescing (??), improving type safety and allowing the TypeScript compiler to catch invalid property paths (per #3604).
Changes:
- Replaced a broad set of
_get()calls with?./??across UI components and utilities. - Added missing type surface area exposed by the migration (Redux
pathAgnosticDecorationsslice and trackingcookiesToDimensionsconfig). - Simplified a few call sites where
_get()had been masking type issues (e.g., GA cookies-to-dimensions iteration).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/jaeger-ui/src/utils/tracking/ga.ts | Removes _get usage for GA config/query access and relies on typed config fields. |
| packages/jaeger-ui/src/utils/plexus/set-on-graph.ts | Replaces _get(zoomTransform, 'k', 1) with zoomTransform?.k ?? 1. |
| packages/jaeger-ui/src/utils/DraggableManager/DraggableManager.ts | Replaces _get(document, 'body.style') with document.body?.style. |
| packages/jaeger-ui/src/types/index.ts | Adds pathAgnosticDecorations to ReduxState. |
| packages/jaeger-ui/src/types/config.ts | Adds tracking.cookiesToDimensions typing to match runtime usage. |
| packages/jaeger-ui/src/reducers/embedded.ts | Removes _get(window, 'location.search') in favor of window.location?.search. |
| packages/jaeger-ui/src/model/path-agnostic-decorations/index.tsx | Replaces _get path lookups with direct indexed access into pathAgnosticDecorations. |
| packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanTreeOffset.tsx | Removes _get and uses relatedTarget type guard + direct dataset access. |
| packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageHeader.tsx | Replaces _get around _maxBy(...) with optional chaining + ??. |
| packages/jaeger-ui/src/components/TracePage/index.tsx | Replaces _get with ?./?? for trace graph vertices and spans access. |
| packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/traceDiffGraphUtils.ts | Replaces _get(..., 'size') with optional chaining on .size. |
| packages/jaeger-ui/src/components/SearchTracePage/SearchForm.tsx | Replaces _get(state, 'config.search.*') with optional chaining. |
| packages/jaeger-ui/src/components/DeepDependencies/traces.tsx | Removes _get(graphState, 'model.hash') in favor of guarded direct access. |
| packages/jaeger-ui/src/components/DeepDependencies/index.tsx | Removes _get usages for hash and DDG state entry access (computed keys). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
|
@IstvanCsVarga Can you please resolve the merge conflicts? |
IstvanCsVarga
force-pushed
the
remove-lodash-get
branch
from
4f5c4e4 to
5ce11ba
Compare
Replaces most lodash _get() calls with native optional chaining (?.) and nullish coalescing (??) operators for type-safe, compiler-checked property access. Kept _get in 3 files where the path is genuinely runtime-dependent (comes from user-configured schemas at runtime): DetailsPanel.tsx, actions/path-agnostic-decorations.ts, utils/config/process-deprecation.ts. Adds missing pathAgnosticDecorations to ReduxState type and cookiesToDimensions to tracking config type, which were previously bypassed by _get()'s lack of type checking. Resolves jaegertracing#3604 Signed-off-by: Istvan Csaba Varga <istvan.csaba.varga@accenture.com>
IstvanCsVarga
force-pushed
the
remove-lodash-get
branch
from
5ce11ba to
fb244d4
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Removes most unsafe lodash/get usages in jaeger-ui in favor of optional chaining / nullish coalescing, improving type-safety and letting TypeScript catch invalid property paths (per #3604).
Changes:
- Replaced many
_get()calls with?./??across UI code paths (tracking, trace pages, DDG, diff graph, search form). - Added missing type surface area exposed by safer property access (
ReduxState.pathAgnosticDecorations,Config.tracking.cookiesToDimensions). - Simplified a few call sites by removing casting/overly generic access patterns once types are in place.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/jaeger-ui/src/utils/tracking/ga.ts | Replaces _get() with optional chaining for GA config + query parsing; uses typed cookiesToDimensions. |
| packages/jaeger-ui/src/utils/plexus/set-on-graph.ts | Replaces _get() defaulting with zoomTransform?.k ?? 1 for match size calculations. |
| packages/jaeger-ui/src/utils/DraggableManager/DraggableManager.ts | Replaces _get(document, 'body.style') with document.body?.style. |
| packages/jaeger-ui/src/types/index.ts | Adds pathAgnosticDecorations slice to ReduxState typing. |
| packages/jaeger-ui/src/types/config.ts | Adds tracking.cookiesToDimensions to config typing. |
| packages/jaeger-ui/src/model/path-agnostic-decorations/index.tsx | Replaces dynamic-path _get() reads with typed slice access + optional chaining. |
| packages/jaeger-ui/src/components/TracePage/index.tsx | Replaces _get() for vertices/spans access in ui-find matching logic. |
| packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanTreeOffset.tsx | Replaces _get(event, 'relatedTarget.dataset…') with a type-guarded local relatedTarget. |
| packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageHeader.tsx | Replaces _get(_maxBy(...), 'depth', 0) with optional chaining + ??. |
| packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/traceDiffGraphUtils.ts | Removes _get() and checks filterSpans(... )?.size directly. |
| packages/jaeger-ui/src/components/SearchTracePage/SearchForm.tsx | Replaces config _get() reads with state.config?.search?.…. |
| packages/jaeger-ui/src/components/DeepDependencies/traces.tsx | Removes _get() for graph hash extraction when sanitizing URL state. |
| packages/jaeger-ui/src/components/DeepDependencies/index.tsx | Removes _get() for DDG hash + state entry access using optional chaining / bracket indexing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Author
|
@Parship12 Done — rebased onto latest main, conflicts resolved, and PR is now a single clean commit. All checks pass and the branch is mergeable. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Resolves #3604
Replaces most
lodash/get(_get()) calls with native JavaScript optional chaining (?.) and nullish coalescing (??) operators for type-safe, compiler-checked property access.Description of the changes
_get(state, 'config.search.maxLookback')->state.config?.search?.maxLookback_get(zoomTransform, 'k', 1)->zoomTransform?.k ?? 1state.ddg?.[getStateEntryKey(...)]_getin the 3 files where paths are genuinely runtime-dependent (DetailsPanel.tsx,path-agnostic-decorations.ts,process-deprecation.ts)Type improvements
Adding type-safe access exposed two missing type definitions that
_get()had been silently bypassing:pathAgnosticDecorationstoReduxStatetypecookiesToDimensionsto the tracking config typeHow was this change tested?
npm run prettier- passnpm run lint(eslint + tsc-lint + prettier-lint + license checks) - passnpm test- all 2565 tests passnpm run build- production build succeedsChecklist
AI Usage in this PR (choose one)
See AI Usage Policy.