Skip to content

Fix: Tickets menu on Changeset pages redirects to make core/reports/ page instead of opening tickets menu#556

Closed
hbhalodia wants to merge 3 commits intoWordPress:trunkfrom
hbhalodia:fix/issue-8183
Closed

Fix: Tickets menu on Changeset pages redirects to make core/reports/ page instead of opening tickets menu#556
hbhalodia wants to merge 3 commits intoWordPress:trunkfrom
hbhalodia:fix/issue-8183

Conversation

@hbhalodia
Copy link

Issue

Meta ticket -- https://meta.trac.wordpress.org/ticket/8183

  • There was a JS error on the page accessing the value from the null.
  • Added a safe check before accessing.

@github-actions
Copy link

github-actions bot commented Mar 3, 2026

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props hbhalodia, dd32.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Comment on lines +1755 to +1758
let match = canonical.match( /\/ticket\/(\d+)$/ );
if ( match && match[1] ) {
ticket = match[1];
}
Copy link
Member

Choose a reason for hiding this comment

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

I was going to suggest using OR, but.. this is pretty ugly with no benefit:

Suggested change
let match = canonical.match( /\/ticket\/(\d+)$/ );
if ( match && match[1] ) {
ticket = match[1];
}
( canonical.match( /\/ticket\/(\d+)$/ ) || [,0] )[1]

Using Nullish operators can also one-line it, which feels much cleaner to me, as it can be easily used on more complicated code without having to nest multiple levels of if checks.
(This ends up as ticket being undefined which is fine for this use-case)

Suggested change
let match = canonical.match( /\/ticket\/(\d+)$/ );
if ( match && match[1] ) {
ticket = match[1];
}
canonical.match( /\/ticket\/(\d+)$/ )?.[0]

(This is what I'm going to merge, you'll still get props for it)

However, in this case, there's no need to check the match value, as if it matches due to the static regex you can be sure it'll be set.

Suggested change
let match = canonical.match( /\/ticket\/(\d+)$/ );
if ( match && match[1] ) {
ticket = match[1];
}
let match = canonical.match( /\/ticket\/(\d+)$/ );
if ( match ) {
ticket = match[1];
}

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @dd32 For review.

Initially I had also used canonical.match( /\/ticket\/(\d+)$/ )?.[0] the nullish operator, but I searched the codebase if optional chaining is used or not for codebase to be consistent, which I ends up using the if checks.

I will update PR with what is being suggested.

Also, we are just matching ticket number instead of whole pattern, I guess it should be canonical.match( /\/ticket\/(\d+)$/ )?.[1]?

Thanks,

@hbhalodia hbhalodia requested a review from dd32 March 4, 2026 04:27
@bazza bazza closed this in 515c0ad Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants