CHARTS-180: Fix duplicate items.find() and falsy value guard in BaseLegend by Copilot · Pull Request #47459 · Automattic/jetpack

Copilot · 2026-03-05T03:35:19Z

Fixes CHARTS-180: Fix double items.find() and truthy check for legend value

Summary

Fixes legend rendering so falsy-but-valid values (notably numeric 0) are displayed, while also avoiding redundant items.find() calls during render. Additionally makes BaseLegendItem.value optional to align the type with runtime behavior and docs, and guards against empty string values rendering empty spans.

Proposed changes:

Extract items.find() result into a matchedItem variable to avoid calling it twice per label on every render.
Replace the truthy ?.value && guard with an explicit ?.value != null && value !== '' check so falsy-but-valid values (e.g. numeric 0) are no longer silently hidden, while empty strings don't produce empty value spans.
Make BaseLegendItem.value optional in the type definition and API reference docs — the component already treated it as optional at runtime, and the docs described it as optional.
Update useLeaderboardLegendItems to omit value entirely instead of setting value: ''.
Add tests for numeric 0 values, empty string values, and missing values.

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

N/A — internal charts library fix.

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Run jp test js js-packages/charts — all legend and leaderboard legend tests should pass.
Open Storybook (jp storybook js-packages/charts) and verify:
- Legend > Default and Legend > Vertical — values (e.g. 65%, 35%) render next to labels.
- Legend > With Line Chart and Legend > With Bar Chart — legends render correctly without values (no empty spans or extra spacing).
- Leaderboard Chart > With Legend — legend items render with labels and colors only, no empty value spans.

Changelog

Changelog entry added at projects/js-packages/charts/changelog/charts-180-fix-duplicate-find-truthy-check-legend.

github-actions · 2026-03-05T03:36:49Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
You can use GitHub's Reviewers functionality to request a review.
When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

Co-authored-by: adamwoodnz <1017872+adamwoodnz@users.noreply.github.com>

adamwoodnz · 2026-03-05T03:43:04Z

@copilot use https://github.com/Automattic/jetpack/blob/trunk/.agents/skills/jetpack-pr.md when creating your PR

github-actions · 2026-03-05T03:44:17Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack), and enable the copilot/fix-duplicate-find-check branch.
To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack copilot/fix-duplicate-find-check

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

Copilot · 2026-03-05T03:44:29Z

@copilot use https://github.com/Automattic/jetpack/blob/trunk/.agents/skills/jetpack-pr.md when creating your PR

Updated the PR description to follow the template from .github/PULL_REQUEST_TEMPLATE.md — includes the standard sections (Proposed changes, Other information, Testing instructions, Changelog).

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

https://api.github.com/graphql
- Triggering command: `/usr/bin/gh gh pr create --title CHARTS-180: Fix duplicate items.find() and falsy value guard in BaseLegend --body Fixes #

Proposed changes:

Extract items.find() result into a matchedItem variable to avoid calling it twice per label on every render.
Replace the truthy ?.value && guard with an explicit ?.value != null check so falsy-but-valid values (e` (http block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled

Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

adamwoodnz · 2026-03-05T03:46:38Z

@claude review please

claude · 2026-03-05T03:46:55Z

Claude finished @adamwoodnz's task in 1m 4s —— View job

Code Review

Gather context
Review changed files
Post review feedback

The changes are correct and well-targeted. Here's my analysis:

`base-legend.tsx`

matchedItem extraction (line 186)
Clean and correct. Avoids two redundant Array.prototype.find calls per label on every render.

?.value != null guard (line 264)
This is the right fix. != null uses loose equality so it excludes both null and undefined, while correctly allowing falsy-but-valid values like 0, "", and false.

One subtle edge case worth noting: value: "" (empty string) and value: false are now treated as displayable values (they weren't before). An empty string will render a span with no visible content:

<span className="legend-item-value">&nbsp;</span>

This is likely fine in practice, but it's a behavior change beyond what the PR description mentions. If the value type in BaseLegendItem is typed as string | number (not boolean), this is a non-issue — worth a quick check.

matchedItem.value (line 268, no optional chaining)
Correct. The conditional already guards that matchedItem?.value != null is truthy, so matchedItem is guaranteed to be defined here.

Pre-existing inconsistency (not introduced by this PR)
The glyph branch uses items[i] (index-based, line 217) while the value display uses matchedItem (label-based). These should be equivalent when labels are unique, but the inconsistency is worth keeping an eye on.

`legend.test.tsx`

The new test (lines 73–81) correctly asserts that both value: 0 (numeric) and value: '0%' (string) are rendered. This directly validates the bug fix.

Minor suggestion: A test asserting that value: null is not rendered would complement the existing value: undefined test and explicitly document the != null behavior:

test( 'does not render null value', () => {
  const items = [ { label: 'Item 1', color: '#ff0000', value: null } ];
  render( <BaseLegend items={ items } orientation="horizontal" /> );
  expect( screen.queryByTestId( 'legend-item-value' ) ).not.toBeInTheDocument();
} );

This is optional — the fix is still correct without it.

Changelog

Format and content look correct for the Jetpack changelogger.

Summary: The fix is correct and the test covers the primary scenario. The only noteworthy point is the implicit behavior change for empty string/false values — if those types aren't in the value field's TypeScript type, it's a non-issue. Otherwise, a comment acknowledging the extended truthiness semantics might be helpful. Good to merge.
|

Copilot

Pull request overview

Fixes legend rendering so falsy-but-valid values (notably numeric 0) are displayed, while also avoiding redundant items.find() calls during render.

Changes:

Store items.find(...) result in a matchedItem variable to avoid duplicate lookups.
Switch the legend value guard from a truthy check to != null so 0 renders.
Add a unit test verifying that value: 0 is rendered.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`projects/js-packages/charts/src/components/legend/private/base-legend.tsx`	Avoids duplicate `find()` and changes the value guard to allow rendering `0`.
`projects/js-packages/charts/src/components/legend/test/legend.test.tsx`	Adds coverage for rendering numeric zero values in the legend.
`projects/js-packages/charts/changelog/charts-180-fix-duplicate-find-truthy-check-legend`	Documents the fix as a patch-level change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

projects/js-packages/charts/src/components/legend/private/base-legend.tsx

jp-launch-control · 2026-03-05T03:59:25Z

Code Coverage Summary

Coverage changed in 1 file.

File	Coverage	Δ%	Δ Uncovered
projects/js-packages/charts/src/components/legend/private/base-legend.tsx	39/39 (100.00%)	0.00%	0 💚

Full summary · PHP report · JS report

The runtime code and docs already treated value as optional, but the type declared it as required. This caused a misleading type guarantee for consumers. Aligns the type and API reference with actual behavior. Made-with: Cursor

adamwoodnz · 2026-03-05T04:03:41Z

@claude again please

claude · 2026-03-05T04:03:57Z

Claude finished @adamwoodnz's task in 49s —— View job

Code Review

Gather context and read changed files
Analyze changes
Post review feedback

The changes are correct, focused, and well-targeted. Here's my analysis of each change:

`base-legend.tsx`

matchedItem extraction (line 186)
Clean fix. Eliminates two redundant Array.prototype.find calls per label on every render. The variable name is clear and the placement (before the return) is appropriate.

?.value != null guard (line 264)
Correct fix. != null uses loose equality, so it excludes both null and undefined while correctly allowing falsy-but-valid values like 0. The follow-up use of matchedItem.value without optional chaining on line 268 is also correct — the conditional already narrows the type.

Pre-existing inconsistency (not introduced by this PR)
The glyph branch uses index-based access items[i] (line 217) while the value display uses label-based matchedItem. These are equivalent as long as labels are unique (guaranteed by the ordinal scale), but worth flagging as a maintenance point.

`types.ts`

Making value optional (value?: number | string) is the right change and directly addresses the Copilot reviewer's comment. This also means the existing "handles missing values" test (which passes value: undefined) is now correctly typed — previously it would have been a TypeScript error.

`legend.test.tsx`

The new test (lines 73–81) correctly covers value: 0 (numeric) and value: '0%' (string zero-as-string). Both were silently hidden before this fix.

Optional addition: A test for value: null would explicitly document the != null boundary, but isn't strictly required — the fix is correct without it.

`stories/index.api.mdx`

Correctly updated to reflect value as optional, kept in sync with the type change. Good.

Changelog

Format and content are correct for the Jetpack changelogger. Significance patch, type fixed — appropriate.

Summary: This is a clean, minimal, correct fix. The core logic change (!= null), the type alignment (value?:), and the test coverage all line up. No blocking issues. Good to merge.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-05T04:07:32Z

projects/js-packages/charts/src/components/legend/private/base-legend.tsx

+										{ matchedItem?.value != null && (
 											<span className={ styles[ 'legend-item-value' ] }>
 												{ '\u00A0' }
-												{ items.find( item => item.label === label.text )?.value }
+												{ matchedItem.value }
 											</span>


The new matchedItem?.value != null guard will render an empty value string ('') as a value, which changes existing behavior. Several legend item generators in this repo intentionally set value: '' when values should be hidden (e.g. when showValues is false), and with this change those items will now render an extra value span containing only a non‑breaking space, affecting layout/spacing.

Consider treating empty string as “no value” (e.g. additionally check matchedItem.value !== ''), or update upstream item generators to omit value/use undefined when no value should be displayed (and keep BaseLegend aligned with that contract).

Copilot · 2026-03-05T04:07:32Z

projects/js-packages/charts/src/components/legend/test/legend.test.tsx

+		expect( screen.getByText( '0' ) ).toBeInTheDocument();
+		expect( screen.getByText( '0%' ) ).toBeInTheDocument();
+	} );
+


The added coverage verifies 0 renders, but it doesn’t cover the existing “hide values by using an empty string” behavior (some code paths set value: '' when values shouldn’t be shown). With the new != null guard, '' would now render an empty value span and can affect layout; adding a test case for value: '' not rendering (or updating fixtures to omit value) would help prevent regressions.

Suggested change

test( 'does not render value span for empty string values', () => {

const itemsWithEmptyValues = [

{ label: 'Item 1', color: '#ff0000', value: '' },

{ label: 'Item 2', color: '#00ff00', value: '' },

];

const { container } = render(

<BaseLegend items={ itemsWithEmptyValues } orientation="horizontal" />

);

expect( container.querySelectorAll( '[data-testid="legend-value"]' ).length ).toBe( 0 );

} );

Empty string values (e.g. from leaderboard legend items) would render a span containing only a non-breaking space, affecting layout. Now BaseLegend also rejects empty strings, and the leaderboard hook omits value entirely instead of setting it to ''. Made-with: Cursor

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-05T04:28:14Z

projects/js-packages/charts/src/components/legend/private/base-legend.tsx

 							const visible = isSeriesVisible( label.text );
 							const handleClick = createClickHandler( label.text );
 							const handleKeyDown = createKeyDownHandler( label.text );
+							const matchedItem = items.find( item => item.label === label.text );


matchedItem is derived via items.find(...) inside labels.map, which keeps this render path at O(n^2) and is still doing extra work every render. Since the component already assumes labels and items are aligned by index (e.g., items[i] is used for glyph rendering), you can use const matchedItem = items[i]; (or const value = items[i]?.value) to avoid any find() and also avoid mismatches if label.text is formatted differently from item.label.

Suggested change

const matchedItem = items.find( item => item.label === label.text );

const matchedItem = items[ i ];

Labels and items are index-aligned via the ordinal scale domain, so items.find() per label was redundant O(n) work each render. Use items[i] directly. Add regression test to ensure value-label pairing stays correct if alignment assumptions change. Made-with: Cursor

Initial plan

6f01787

Copilot AI assigned Copilot and adamwoodnz Mar 5, 2026

Copilot started work on behalf of adamwoodnz March 5, 2026 03:35 View session

github-actions bot added the [Status] In Progress label Mar 5, 2026

github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 5, 2026

CHARTS-180: Fix duplicate items.find() and truthy check for legend value

fc4c26a

Co-authored-by: adamwoodnz <1017872+adamwoodnz@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Fix duplicate items.find() and update truthy check for legend value~~ CHARTS-180: Fix duplicate items.find() and falsy value guard in BaseLegend Mar 5, 2026

Copilot finished work on behalf of adamwoodnz March 5, 2026 03:40

Copilot started work on behalf of adamwoodnz March 5, 2026 03:43 View session

github-actions bot added [JS Package] Charts [Tests] Includes Tests RNA labels Mar 5, 2026

Copilot finished work on behalf of adamwoodnz March 5, 2026 03:44

adamwoodnz marked this pull request as ready for review March 5, 2026 03:46

Copilot AI review requested due to automatic review settings March 5, 2026 03:46

Copilot started reviewing on behalf of adamwoodnz March 5, 2026 03:46 View session

adamwoodnz added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] In Progress labels Mar 5, 2026

github-actions bot added the [Status] In Progress label Mar 5, 2026

Copilot AI reviewed Mar 5, 2026

View reviewed changes

projects/js-packages/charts/src/components/legend/private/base-legend.tsx Outdated Show resolved Hide resolved

fix(charts): Make BaseLegendItem.value optional

62104ff

The runtime code and docs already treated value as optional, but the type declared it as required. This caused a misleading type guarantee for consumers. Aligns the type and API reference with actual behavior. Made-with: Cursor

adamwoodnz requested a review from Copilot March 5, 2026 04:03

Copilot started reviewing on behalf of adamwoodnz March 5, 2026 04:04 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

adamwoodnz requested a review from Copilot March 5, 2026 04:23

Copilot started reviewing on behalf of adamwoodnz March 5, 2026 04:23 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

+	test( 'does not render value span for empty string values', () => {
+		const itemsWithEmptyValues = [
+			{ label: 'Item 1', color: '#ff0000', value: '' },
+			{ label: 'Item 2', color: '#00ff00', value: '' },
+		];
+		const { container } = render(
+			<BaseLegend items={ itemsWithEmptyValues } orientation="horizontal" />
+		);
+		expect( container.querySelectorAll( '[data-testid="legend-value"]' ).length ).toBe( 0 );
+	} );

	const matchedItem = items.find( item => item.label === label.text );
	const matchedItem = items[ i ];

Conversation

Copilot AI commented Mar 5, 2026 • edited by adamwoodnz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Changelog

Uh oh!

github-actions bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamwoodnz commented Mar 5, 2026

Uh oh!

github-actions bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

I tried to connect to the following addresses, but was blocked by firewall rules:

Proposed changes:

Uh oh!

adamwoodnz commented Mar 5, 2026

Uh oh!

claude bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

base-legend.tsx

legend.test.tsx

Changelog

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

jp-launch-control bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Summary

Uh oh!

adamwoodnz commented Mar 5, 2026

Uh oh!

claude bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

base-legend.tsx

types.ts

legend.test.tsx

stories/index.api.mdx

Changelog

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Mar 5, 2026 •

edited by adamwoodnz

Loading

github-actions bot commented Mar 5, 2026 •

edited

Loading

github-actions bot commented Mar 5, 2026 •

edited

Loading

Copilot AI commented Mar 5, 2026 •

edited

Loading

claude bot commented Mar 5, 2026 •

edited

Loading

`base-legend.tsx`

`legend.test.tsx`

jp-launch-control bot commented Mar 5, 2026 •

edited

Loading

claude bot commented Mar 5, 2026 •

edited

Loading

`base-legend.tsx`

`types.ts`

`legend.test.tsx`

`stories/index.api.mdx`