Link Control: Clear entity metadata when selecting custom URLs #73825
Conversation
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
…ting a page link to a custom link
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +208 B (+0.01%) Total Size: 2.58 MB
ℹ️ View Unchanged
|
jeryj
added
[Type] Bug
|
Flaky tests detected in a78255e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20105672827
|
There was a problem hiding this comment.
Pull request overview
This PR addresses an issue where entity metadata (type/kind) persists when transitioning from entity-bound links (pages, posts) to custom URLs in navigation links. The fix involves refactoring the onChange handler into a dedicated hook and adding defensive checks in LinkControl to explicitly clear entity metadata.
- Refactors link change logic into a reusable
useHandleLinkChangehook with special handling for entity-to-custom transitions - Updates LinkControl to detect and clear entity metadata when custom URLs are selected
- Adds comprehensive unit and E2E test coverage
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/block-library/src/navigation-link/shared/use-handle-link-change.js |
New hook that encapsulates link change logic with binding management and special handling for entity-to-custom transitions |
packages/block-library/src/navigation-link/shared/test/use-handle-link-change.test.js |
Comprehensive unit tests for the new hook covering entity links, custom links, and transitions |
packages/block-library/src/navigation-link/shared/index.js |
Exports the new useHandleLinkChange hook |
packages/block-library/src/navigation-link/edit.js |
Refactored to use the new hook instead of inline onChange handler |
packages/block-editor/src/components/link-control/index.js |
Adds isCustomURLType helper and explicit clearing of entity metadata for custom URL selections |
packages/block-editor/src/components/link-control/test/index.js |
New test verifying entity metadata is cleared when selecting custom URLs via suggestions |
test/e2e/specs/editor/blocks/navigation.spec.js |
E2E test validating the complete user flow of unsyncing and editing bound links |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Check if transitioning from entity to custom link | ||
| const willBeCustomLink = ! updatedLink.id && hasUrlBinding; |
There was a problem hiding this comment.
The condition ! updatedLink.id doesn't correctly detect custom links. According to the test at line 2898 of packages/block-editor/src/components/link-control/test/index.js, custom URL suggestions have an id property set to the URL value (e.g., id: 'https://custom-url.com'). This means willBeCustomLink will always be false when a custom URL suggestion is selected, and the special transition logic (lines 54-65) will never execute.
The test at lines 333-380 uses a mock without an id property, which doesn't match the actual behavior of LinkControl's onChange callback.
Consider detecting custom links by checking the type instead:
const isCustomType = updatedLink.type &&
['link', 'URL', 'mailto', 'tel', 'internal'].includes(updatedLink.type);
const willBeCustomLink = isCustomType && hasUrlBinding;
Suggested change
| const willBeCustomLink = ! updatedLink.id && hasUrlBinding; | |
| const isCustomType = | |
| updatedLink.type && | |
| [ 'link', 'URL', 'mailto', 'tel', 'internal' ].includes( updatedLink.type ); | |
| const willBeCustomLink = isCustomType && hasUrlBinding; |
| * @param {Object} options - Configuration options | ||
| * @param {string} options.clientId - Block client ID | ||
| * @param {Object} options.attributes - Current block attributes | ||
| * @param {Function} options.setAttributes - Standard setAttribute function |
There was a problem hiding this comment.
The parameter documentation refers to "Standard setAttribute function" but should be "Standard setAttributes function" (plural) to match the actual parameter name.
Suggested change
| * @param {Function} options.setAttributes - Standard setAttribute function | |
| * @param {Function} options.setAttributes - Standard setAttributes function |
|
|
||
| // Helper to check if a link value has a custom URL type (not an entity type) | ||
| const isCustomURLType = ( linkValue ) => | ||
| LINK_ENTRY_TYPES.includes( linkValue?.type ); |
There was a problem hiding this comment.
The isCustomURLType function checks if linkValue?.type is in LINK_ENTRY_TYPES, which contains lowercase values like 'link'. However, URL suggestions from search results use uppercase 'URL' as their type (see test at line 2837 and other existing tests). This case mismatch means custom URL selections won't be properly detected, and entity metadata (type/kind) won't be cleared as intended.
Consider making the comparison case-insensitive:
const isCustomURLType = ( linkValue ) =>
LINK_ENTRY_TYPES.includes( linkValue?.type ) || linkValue?.type === 'URL';Or normalize the type to lowercase before checking:
const isCustomURLType = ( linkValue ) =>
LINK_ENTRY_TYPES.includes( linkValue?.type?.toLowerCase() );
Suggested change
| LINK_ENTRY_TYPES.includes( linkValue?.type ); | |
| LINK_ENTRY_TYPES.includes( linkValue?.type?.toLowerCase() ); |
Contributor
Author
There was a problem hiding this comment.
No, the 'URL' was only in the unit tests. I updated that to be removed.
getdave
approved these changes
Dec 9, 2025
Contributor
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
Thanks for following up on this.
I swapped from entity to static URL and saw
<!-- wp:navigation-link {"label":"Nihil ipsam cupiditate voluptates quidem","type":"custom","url":"www.wordpress.org","kind":"custom"} /-->
This looks correct to me. No bindings and nothing else hanging around.
I did notice a potential legacy issue with code and tests having different type values for custom links.
The tests seem to use 'URL' (uppercase) as the type but the actual code produces type: link (lowercase) via URL_TYPE = 'link' in constants. I wonder if this might be causing other issues?
Comment on lines
216
to
219
| const isEntity = | ||
| handleEntities && | ||
| !! internalControlValue?.id && | ||
| ! isCustomURLType( internalControlValue ); |
Contributor
There was a problem hiding this comment.
Ah so custom links had an id 🤦
Contributor
Author
There was a problem hiding this comment.
In the unit tests, the custom links were getting set with an id value of the url.
Comment on lines
310
to
311
| // Explicitly set type and kind to undefined for custom links | ||
| ...( isCustomLink && { type: undefined, kind: undefined } ), |
Contributor
There was a problem hiding this comment.
Just wondering if we should unset id here also?
Contributor
Author
There was a problem hiding this comment.
I wasn't sure. I was hesitant to unset an ID since it previously had one. I figured I would let the ID do what it was before rather than introduce a potential breaking change.
Contributor
There was a problem hiding this comment.
I don't like that the tests don't reflect the code. It could mean we are masking issues...
Contributor
Author
I noticed this too. I wasn't sure if it was intentional to use 'URL' or not, but the tests pass with the new changes so I assumed it was fine. |
…tions Prevents old entity metadata from persisting when changing links. When a new suggestion is selected, only the new suggestion's kind and type are used, not the values from the previous link.
Contributor
Author
|
@getdave I made a few more changes and wanted additional confidence before merging. Specifically this commit. I believe this was a root bug that was passing the old |
getdave
approved these changes
Dec 10, 2025
Contributor
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
This seems to be testing well for me.
I did notice that when using LinkControl in paragraph content we have a similar issue with attributes not being cleared on the link format when a link changes from entity -> custom.
Could we apply a similar fix there?
gutenberg/packages/format-library/src/link/inline.js
Lines 128 to 138 in 27a5744
The issue is nearly always when we merge the new attributes with the old attributes.
| ( suggestion && Object.keys( suggestion ).length >= 1 ) | ||
| ) { | ||
| const { id, url, ...restLinkProps } = currentLink ?? {}; | ||
| const { id, url, kind, type, ...restLinkProps } = |
Contributor
There was a problem hiding this comment.
Let's comment on what we're doing and why here.
Contributor
Author
There was a problem hiding this comment.
Updated. This is actually the root bug fix. I'm going to just bring this change in, as the others are more heavy-handed about forcing the shape of custom url data and could be a breaking change if someone is expecting a certain return value.
| } | ||
| ); | ||
|
|
||
| it( 'should return undefined kind and type when submitting a custom URL', async () => { |
Contributor
There was a problem hiding this comment.
I'm thinking the test should be the control already has a link with kind and type and then we change it and assert that the kind and type are no longer present.
Contributor
Author
There was a problem hiding this comment.
That test is later on: it( 'should clear entity metadata (type/kind) when changing from page link to custom link via suggestion',
|
|
||
| // Helper to check if a link value has a custom URL type (not an entity type) | ||
| const isCustomURLType = ( linkValue ) => | ||
| LINK_ENTRY_TYPES.includes( linkValue?.type ); |
|
|
||
| // Compute isEntity internally based on handleEntities prop and presence of ID | ||
| const isEntity = handleEntities && !! internalControlValue?.id; | ||
| // Exclude custom URLs which have id but type is 'link', 'mailto', 'tel', or 'internal' |
Contributor
There was a problem hiding this comment.
Suggested change
| // Exclude custom URLs which have id but type is 'link', 'mailto', 'tel', or 'internal' | |
| // Exclude custom URLs which (for historical reasons) may have | |
| // an `id` but type is 'link', 'mailto', 'tel', or 'internal' |
| onChange( { | ||
| ...internalControlValue, | ||
| ...nonSettingsChanges, | ||
| // Explicitly set type and kind to undefined for custom links |
Contributor
There was a problem hiding this comment.
Suggested change
| // Explicitly set type and kind to undefined for custom links | |
| // Explicitly unset attributes for custom links to ensure consistency | |
| // and avoid edge cases. |
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.
Splitting out work from #73731
What?
Closes #73767, but I think there may be a further underlying issue there.
Fixes entity metadata (type/kind) persisting when changing from page links to custom URLs.
Why?
Changing to from an entity-bound link to a custom link would persist the kind/type from the previously bound link, which causes odd behavior due to unexpectedly combined values.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast