Skip to content

Conversation

@Parship999
Copy link

Which problem is this PR solving?

The LabeledList component currently returns an array from its map function without proper React Fragment keys.

Issues:

  • The divider element doesn't have a guaranteed unique key when it exists
  • Arrays should be wrapped in Fragments with proper keys

Description of the changes

  • Wraped array elements in React.Fragment with proper key attribution
  • Move the key prop from the <li> element to the React.Fragment

Checklist

@Parship999 Parship999 requested a review from a team as a code owner May 24, 2025 15:13
@Parship999 Parship999 requested review from mahadzaryab1 and removed request for a team May 24, 2025 15:13
@yurishkuro
Copy link
Member

I don't understand what problem this solves.

@Parship999
Copy link
Author

I don't understand what problem this solves.

This PR fixes a react best practices violation that can cause console warning. current code returns a raw array from the map function. React documentation recommends wrapping array returns in Fragments with proper keys

@yurishkuro yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label May 24, 2025
@codecov
Copy link

codecov bot commented May 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.03%. Comparing base (c4cfc67) to head (37cf1c8).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2812   +/-   ##
=======================================
  Coverage   97.03%   97.03%           
=======================================
  Files         256      256           
  Lines        7947     7947           
  Branches     2003     2055   +52     
=======================================
  Hits         7711     7711           
  Misses        236      236           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yurishkuro yurishkuro enabled auto-merge May 24, 2025 19:30
@yurishkuro yurishkuro added this pull request to the merge queue May 24, 2025
Merged via the queue into jaegertracing:main with commit 022b068 May 24, 2025
10 of 11 checks passed
@Parship999 Parship999 deleted the fix-labeledlist-array-return branch May 24, 2025 19:38
github-merge-queue bot pushed a commit that referenced this pull request May 31, 2025
## Which problem is this PR solving?
While working on some other issues in the codebase, I found additional
components with similar React Fragment and key issues. These components
had the same React Fragment and key patterns (like in #2812 ) that
needed to be solved for consistency and react best practices

## Description of the changes
1. `KeyValuesTable.tsx`
- Changed `<>` to `<React.Fragment key={i}>` for proper key attribution
2. `highlightMatches.tsx`
- Ensured consistent return types by wrapping plain strings in `<span>`
elements
- Added proper keys to all elements returned from map function
3. `SearchResults/index.tsx`
- Added keys to `React.Fragment` elements in conditional rendering

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

---------

Signed-off-by: Parship Chowdhury <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants