Part of Phase 1 (1f): remove config Redux slice; use getConfig via useConfig#3781
Open
Parship12 wants to merge 2 commits intojaegertracing:mainfrom
Open
Part of Phase 1 (1f): remove config Redux slice; use getConfig via useConfig#3781Parship12 wants to merge 2 commits intojaegertracing:mainfrom
Parship12 wants to merge 2 commits intojaegertracing:mainfrom
Conversation
Signed-off-by: Parship Chowdhury <parshipchowdhury@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3781 +/- ##
==========================================
- Coverage 90.58% 90.58% -0.01%
==========================================
Files 334 333 -1
Lines 10483 10478 -5
Branches 2737 2736 -1
==========================================
- Hits 9496 9491 -5
Misses 860 860
Partials 127 127 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is part of the UI state-management migration (Phase 1, item 1f), removing the Redux config slice and switching config access to the useConfig() hook (backed by getConfig()), plus updating affected tests and ADR status.
Changes:
- Remove
configfrom the Redux root state typing and reducer set (deletereducers/configand its test). - Update
useConfig()to returngetConfig()directly, and replace Redux-based hook tests withgetConfig-mock-based tests. - Rewire/selectively update component tests and docs to reflect the removal of the Redux
configslice.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/jaeger-ui/src/types/index.ts | Removes config from ReduxState typing. |
| packages/jaeger-ui/src/reducers/index.ts | Removes config reducer from the root reducer map. |
| packages/jaeger-ui/src/reducers/config.ts | Deletes the Redux config slice reducer. |
| packages/jaeger-ui/src/reducers/config.test.ts | Deletes tests for the removed reducer. |
| packages/jaeger-ui/src/hooks/useConfig.ts | Switches hook implementation from Redux selector to getConfig(). |
| packages/jaeger-ui/src/hooks/useConfig.test.tsx | Removes Redux-store-based hook tests. |
| packages/jaeger-ui/src/hooks/useConfig.test.ts | Adds getConfig()-mock-based hook tests. |
| packages/jaeger-ui/src/components/TracePage/index.test.jsx | Updates mapStateToProps tests to remove config usage (but introduces an inconsistency). |
| packages/jaeger-ui/src/components/SearchTracePage/index.test.jsx | Updates tests to mock config via get-config instead of a Redux config slice. |
| packages/jaeger-ui/src/components/App/TopNav.tsx | Starts using useConfig() for menu config (still mixes in direct getConfig() calls). |
| packages/jaeger-ui/src/components/App/TopNav.test.jsx | Updates tests to mock useConfig() and resets mocks appropriately. |
| docs/adr/0004-state-management-strategy.md | Marks phases 1e and 1f as completed. |
Comments suppressed due to low confidence (1)
packages/jaeger-ui/src/components/TracePage/index.test.jsx:1133
mapStateToProps()is called twice with the samestate/ownProps, but the tests now assert two different exact shapes: one expects only{ id, trace }and the new one expects{ id, uiFind: undefined, trace }. SincemapStateToPropsspreadsextractUiFindFromState(state)(which returns{ uiFind: undefined }when no query param is present), at least one of these expectations will fail. Update the assertions to be consistent (e.g., includeuiFindin the expected object or useexpect.objectContaining).
it('returns the correct props shape', () => {
const props = mapStateToProps(state, ownProps);
expect(props).toEqual({
id: traceID,
uiFind: undefined,
trace: { data: {}, state: fetchedState.DONE },
});
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Parship Chowdhury <parshipchowdhury@gmail.com>
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?
getConfig()), so keeping it in redux duplicate that source of truth without any actions or updates.getConfig()first run. A Zustand store would add subscription/update machinery with no set path, so like written in ADR “Zustand or module constant” option is satisfied bygetConfig()+useConfig().Description of the changes
useConfig()now returngetConfig()directly instead ofuseSelector(state => state.config).reducers/config.tsand its test, removedconfigfrom the root reducer map and fromReduxState.How was this change tested?
Checklist
make lint testAI Usage in this PR (choose one)
See AI Usage Policy.