-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PCH Smart Linking: Use the post title as the Smart Link title. #2650
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SmartLink
participant Post
User ->> SmartLink: new SmartLink($destination_post_id)
SmartLink ->> Post: get_the_title($destination_post_id)
Post -->> SmartLink: post_title
SmartLink ->> User: return SmartLink object with title set to post_title
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this 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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- src/Models/class-smart-link.php (1 hunks)
Additional context used
Path-based instructions (1)
src/Models/class-smart-link.php (1)
Pattern
**/*.{html,php}
: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the HTML and PHP code to ensure it is well-structured and adheres to best practices.
- Ensure the code follows WordPress coding standards and is well-documented.
- Confirm the code is secure and free from vulnerabilities.
- Optimize the code for performance, removing any unnecessary elements.
- Validate comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Verify code compatibility with the latest version of WordPress, avoiding deprecated functions or features."
Additional comments not posted (1)
src/Models/class-smart-link.php (1)
132-137
: Dynamic title setting based on destination post IDThe modification to dynamically set the title based on the destination post ID is a significant improvement for usability and relevance of the smart links. This change aligns with the PR objectives to enhance the Smart Link title quality.
- Correctness: The use of
get_the_title
is appropriate here as it fetches the title of a post by its ID, which is exactly what's needed.- Security: Ensure that the title fetched is properly escaped when displayed to prevent XSS vulnerabilities, especially if it's output in HTML context.
- Performance: Consider caching the title if it's fetched frequently to reduce database queries, especially on high-traffic sites.
- Best Practice: The conditional check
0 !== $this->destination_post_id
is a strict comparison, which is good for avoiding type juggling issues.
fe88f4f
to
27f1d07
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, makes sense. Does this qualify as a change or a fix?
Good question @acicovic ! I would say a change, since it wasn't really broken before, and this is more of an improvement than a fix. But I can also see why it could be a fix. Feel free to change the tag if you think it makes more sense as a fix 🙁 |
Since we're releasing a patch release and this can be flagged as a fix, I'm changing the tag to be a fix. |
Description
This PR improves the Smart Link title returned by the Content Suggestions API. Previously, the title was retrieved from the post title stored in the vector database, which is in lowercase. Now, the Smart Link will use the destination post title if a valid post object is found using the canonical URL/permalink.
Motivation and context
Improve the quality of the Smart Links attributes.
How has this been tested?
Tested locally.
Summary by CodeRabbit