Skip to content

Conversation

SebastianWiz
Copy link
Contributor

Context

⛑️ Ticket(s): https://secure.helpscout.net/conversation/3102878428/90380?viewId=3808239

Summary

This PR fixes an issue where using the Add New snippet with GPPA would display the "No Results" text as – No Results – instead of — No Results —.

Furthermore the PR also adds a new hide_no_results_text option. Quick loom showing the option: https://www.loom.com/share/534848f926554e4086b5d6d0b5459951

Copy link

coderabbitai bot commented Oct 8, 2025

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title includes the file name in backticks and a colon, which adds unnecessary noise and verbosity beyond summarizing the core change. It also attempts to cover both the HTML entity fix and the new hide option in one long phrase, reducing clarity and conciseness. A pull request title should be a single, focused sentence that highlights the primary update without file paths or formatting markup. Rename the title to a concise summary such as “Fix HTML entity display in no results text and add hide_no_results_text option,” removing the file name and backticks to improve clarity and brevity.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed The pull request description follows the repository template by including a Context section with the ticket link and a Summary section that clearly explains both the HTML entity correction and the new hide_no_results_text option, and it has removed unused template lines. All required sections are present and sufficiently detailed, and the author provides relevant links without extraneous placeholders. The description aligns well with the provided template and covers the key information.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch seb/fix/90380-html-entities-label

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

github-actions bot commented Oct 8, 2025

Warnings
⚠️ When ready, don't forget to request reviews on this pull request from your fellow wizards.

Generated by 🚫 dangerJS against 082c866

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
gp-advanced-select/gpadvs-enable-add-new-option.php (1)

318-327: LGTM! HTML entity decoding fixes the display issue.

The implementation correctly handles both cases:

  • When hide_no_results_text is true: removes the placeholder choice entirely (line 320)
  • When hide_no_results_text is false: preserves the placeholder but decodes HTML entities (line 325), fixing the issue where – was displayed literally instead of

The defensive check on line 324 ensures the text key exists before decoding.

Consider specifying the ENT_QUOTES flag explicitly for clarity, though the default behavior is sufficient:

 if ( isset( $choices[0]['text'] ) ) {
-    $choices[0]['text'] = html_entity_decode( $choices[0]['text'] );
+    $choices[0]['text'] = html_entity_decode( $choices[0]['text'], ENT_QUOTES );
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7e9bf7 and 082c866.

📒 Files selected for processing (1)
  • gp-advanced-select/gpadvs-enable-add-new-option.php (5 hunks)
🔇 Additional comments (4)
gp-advanced-select/gpadvs-enable-add-new-option.php (4)

28-28: LGTM! New argument added correctly.

The new hide_no_results_text argument with a default value of false maintains backward compatibility while enabling the new functionality.


88-94: LGTM! JavaScript implementation is sound.

The logic correctly initializes the settings.render object if needed and sets no_results to null to hide the message when the option is enabled.


125-125: LGTM! Argument correctly passed to JavaScript.

The PHP argument is properly converted to camelCase convention and passed to the JavaScript constructor.


338-338: LGTM! Example usage documents the new option.

The commented example demonstrates the new hide_no_results_text option, helping users understand how to enable it.

@SebastianWiz SebastianWiz merged commit 0c6e5c0 into master Oct 10, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants