Skip to content

[Action Bar] Fix warning with menu items - #6935

Merged
pksjce merged 8 commits into
mainfrom
jellobagel-action-bar
Oct 8, 2025
Merged

[Action Bar] Fix warning with menu items#6935
pksjce merged 8 commits into
mainfrom
jellobagel-action-bar

Conversation

@JelloBagel

@JelloBagelJelloBagel commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

Related to https://github.com/github/github-ui/pull/3638#discussion_r2395195779

Fixed Each child in a list should have a unique "key" prop warning when mapping the menu items

Console log showing the warning

Changelog

Added

  • Added a header right to the ActionBar's Comment Box story

Changed

  • Update key to use ariaLabel instead of undefined menuItemChildren

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@changeset-bot

changeset-botBot commented Oct 1, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4402b7a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
NameType
@primer/reactPatch
@primer/styled-reactPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actionsgithub-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Oct 1, 2025
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

@github-actions
github-actionsBottemporarily deployed to storybook-preview-6935 October 1, 2025 20:56 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-6935 October 1, 2025 21:04 Inactive
@JelloBagelJelloBagel changed the title [Action Bar] Fix overflow calculations of more menu button[Action Bar] Fix warning with menu itemsOct 1, 2025
@JelloBagel
JelloBagelforce-pushed the jellobagel-action-bar branch from 3710770 to 4ba828bCompareOctober 1, 2025 21:18
@github-actions
github-actionsBottemporarily deployed to storybook-preview-6935 October 1, 2025 21:27 Inactive
@JelloBagelJelloBagel added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Oct 1, 2025
@github-actionsgithub-actionsBot added integration-tests: failing Changes in this PR cause breaking changes in gh/gh and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Oct 1, 2025
@github-actions
github-actionsBottemporarily deployed to storybook-preview-6935 October 1, 2025 22:44 Inactive
@JelloBagelJelloBagel added update snapshots 🤖 Command that updates VRT snapshots on the pull request and removed update snapshots 🤖 Command that updates VRT snapshots on the pull request labels Oct 1, 2025
@JelloBagel
JelloBagel marked this pull request as ready for review October 1, 2025 23:35
@JelloBagel
JelloBagel requested a review from a team as a code ownerOctober 1, 2025 23:35

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a React warning about missing unique "key" props in ActionBar menu items by changing the key from menuItemChildren (which was undefined) to ariaLabel. It also enhances the ActionBar Comment Box story with header right content.

  • Fixed React key prop warning by using ariaLabel instead of undefined menuItemChildren for menu item keys
  • Added header right section to the Comment Box story with Write/Preview buttons
  • Updated e2e test expectations to match the new button count in the story

Reviewed Changes

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

Show a summary per file
FileDescription
packages/react/src/ActionBar/ActionBar.tsxFixed key prop by using ariaLabel and removed unused menuItemChildren destructuring
packages/react/src/ActionBar/ActionBar.examples.stories.tsxAdded header right section with Write/Preview buttons
packages/react/src/ActionBar/ActionBar.examples.stories.module.cssAdded CSS styles for the new header right section
e2e/components/drafts/ActionBar.test.tsUpdated test expectations to match new button counts
.changeset/smart-hornets-happen.mdAdded changeset for patch release

return (
<ActionList.Item
key={menuItemChildren}
key={ariaLabel}

CopilotAIOct 1, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ariaLabel as the key could cause issues if multiple menu items have the same aria-label or if aria-label is undefined. Consider using index or a combination of index and ariaLabel to ensure uniqueness.

Suggested change
key={ariaLabel}
key={`${index}-${ariaLabel??'item'}`}

Copilot uses AI. Check for mistakes.
@primer
primerBot requested a review from a team as a code ownerOctober 1, 2025 23:37
@primer
primerBot requested a review from langermankOctober 1, 2025 23:37
@github-actionsgithub-actionsBot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Oct 1, 2025
@github-actions
github-actionsBottemporarily deployed to storybook-preview-6935 October 1, 2025 23:47 Inactive
return (
<ActionList.Item
key={menuItemChildren}
key={ariaLabel}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make it related to index ? Would index be changing between renders somehow?

@JelloBagelJelloBagelOct 2, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The index could change if the window is resized. For example, if the toolbar overflowed at the "Insert Link" button, then it would be the first index in the menu and the key would be "0". When the window is resized smaller to overflow the next button, then the "Insert Code" button would be the first index now with a key of "0". While each key is still unique, this can cause odd issues if the keys match different components when rerendering in React

@primer-integration

Copy link
Copy Markdown

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/3796

@primer-integration

Copy link
Copy Markdown

🔴 ci completed with status failure.

@JelloBagel
JelloBagel added this pull request to the merge queueOct 6, 2025
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to failed status checks Oct 6, 2025
@github-actions
github-actionsBottemporarily deployed to storybook-preview-6935 October 8, 2025 04:15 Inactive
@pksjcepksjce added the integration-tests: skipped manually Changes in this PR do not require an integration test label Oct 8, 2025
@pksjce
pksjce added this pull request to the merge queueOct 8, 2025
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to failed status checks Oct 8, 2025
@pksjce
pksjce added this pull request to the merge queueOct 8, 2025
Merged via the queue into main with commit 8641c7aOct 8, 2025
45 checks passed
@pksjce
pksjce deleted the jellobagel-action-bar branch October 8, 2025 05:30
@primerprimerBot mentioned this pull request Oct 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: failingChanges in this PR cause breaking changes in gh/ghintegration-tests: skipped manuallyChanges in this PR do not require an integration test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@JelloBagel@pksjce