Add Traffic Boost settings infrastructure #2942
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant enhancements to the dashboard and settings management by adding new features and modifying existing structures. Key changes include the introduction of new methods in the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@acicovic has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (6)
src/content-helper/common/settings/types/index.ts (1)
30-30: Enhance the comment documentation above the Settings type.The current comment "Generic type for settings" could be more descriptive to better explain the purpose and usage of this union type.
Consider updating it to:
-// Generic type for settings. +/** + * Union type representing all available settings types in the application. + * + * @since 3.14.0 + */src/content-helper/common/utils/permissions.ts (1)
Line range hint
14-19: Update JSDoc to document recent changes.The JSDoc comment should be updated to include a
@sincetag for the latest changes related to Traffic Boost functionality./** * Returns the current user's permissions for the Content Helper. * * @since 3.16.0 + * @since 3.17.0 Added Traffic Boost permissions. * * @return {ContentHelperPermissions} The current user's permissions. */src/rest-api/settings/class-endpoint-traffic-boost-settings.php (1)
13-20: Enhance class documentation with more details about Traffic Boost featureWhile the documentation follows WordPress standards, consider adding:
- Purpose of Traffic Boost feature
- Description of settings that will be managed
- Example usage or expected behavior
src/content-helper/dashboard-page/dashboard-page.tsx (2)
55-61: Consider improving error handling and documentationWhile the code works, there are several improvements that could be made:
- Add error handling for cases where the element doesn't exist
- Consider using React refs instead of direct DOM manipulation
- Add JSDoc param documentation for better maintainability
Here's a suggested improvement:
/** * Replaces the first link to have the hash router link. * + * @since 3.18.0 + * @throws {Error} When the first link element cannot be found. */ useEffect(() => { const firstLink = document.querySelector( '#toplevel_page_parsely-dashboard-page .wp-submenu li a.wp-first-item' ); - if (firstLink) { - firstLink.setAttribute( - 'href', - window.location.pathname + window.location.search + '#/' - ); - } + if (!firstLink) { + console.warn('First link element not found in the submenu.'); + return; + } + + firstLink.setAttribute( + 'href', + window.location.pathname + window.location.search + '#/' + ); }, []);
Line range hint
71-84: Enhance type safety and error handlingThe submenu highlighting logic could benefit from:
- Type safety improvements
- Better error handling
- Performance optimization
- React-style DOM handling
Here's a suggested improvement:
/** * Changes the submenus highlight based on the current page. * * @since 3.18.0 + * @throws {Error} When submenu elements cannot be found. */ useEffect(() => { const submenuItems = document.querySelectorAll( '#toplevel_page_parsely-dashboard-page .wp-submenu li' ); + if (!submenuItems.length) { + console.warn('No submenu items found.'); + return; + } - submenuItems.forEach((item) => { - const link = item.querySelector('a'); - const hashPath = link?.getAttribute('href')?.split('#')[1]; + Array.from(submenuItems).forEach((item: Element) => { + const link = item.querySelector('a') as HTMLAnchorElement | null; + if (!link?.href) return; + const hashPath = new URL(link.href).hash.slice(1); if (hashPath === location.pathname) { item.classList.add('current'); link?.blur(); } else { item.classList.remove('current'); } }); }, [location]);src/UI/class-dashboard-page.php (1)
194-217: Consider caching the Traffic Boost settings.Making a REST API request on every dashboard page load could impact performance. Consider implementing transient caching for the settings.
Example implementation:
protected function inject_traffic_boost_settings(): void { + $cache_key = 'parsely_traffic_boost_settings'; + $settings = get_transient($cache_key); + + if (false === $settings && !defined('INTEGRATION_TESTS_RUNNING')) { + $settings = rest_do_request( + new WP_REST_Request( + 'GET', + '/wp-parsely/v2/settings/traffic-boost' + ) + )->get_data(); + + if (is_array($settings)) { + set_transient($cache_key, $settings, HOUR_IN_SECONDS); + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
build/content-helper/dashboard-page.asset.phpis excluded by!build/**build/content-helper/dashboard-page.jsis excluded by!build/**build/content-helper/editor-sidebar.asset.phpis excluded by!build/**build/content-helper/editor-sidebar.jsis excluded by!build/**
📒 Files selected for processing (8)
src/UI/class-dashboard-page.php(3 hunks)src/content-helper/common/settings/types/index.ts(2 hunks)src/content-helper/common/settings/types/traffic-boost-settings.d.ts(1 hunks)src/content-helper/common/utils/permissions.ts(2 hunks)src/content-helper/dashboard-page/dashboard-page.tsx(3 hunks)src/content-helper/dashboard-page/pages/dashboard/page-component.tsx(1 hunks)src/rest-api/settings/class-endpoint-traffic-boost-settings.php(1 hunks)src/rest-api/settings/class-settings-controller.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/UI/class-dashboard-page.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."
src/content-helper/common/settings/types/index.ts (1)
Pattern **/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/content-helper/common/settings/types/traffic-boost-settings.d.ts (1)
Pattern **/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/content-helper/common/utils/permissions.ts (1)
Pattern **/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/content-helper/dashboard-page/dashboard-page.tsx (1)
Pattern **/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/content-helper/dashboard-page/pages/dashboard/page-component.tsx (1)
Pattern **/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/rest-api/settings/class-endpoint-traffic-boost-settings.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."
src/rest-api/settings/class-settings-controller.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 (11)
src/content-helper/common/settings/types/traffic-boost-settings.d.ts (1)
1-5: LGTM! Well-documented with proper versioning.
The JSDoc comment block follows WordPress coding standards and includes the required @SInCE tag with the correct future version.
src/content-helper/common/settings/types/index.ts (1)
13-13: LGTM! Verify the referenced type definition file.
The changes follow TypeScript best practices and maintain consistency with the existing code structure.
Let's verify that the referenced type definition file exists:
Also applies to: 26-26, 30-30
✅ Verification successful
Verification Successful: The TrafficBoostSettings type definition file exists and correctly exports the interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the traffic-boost-settings.d.ts file
# and check its content structure
# Check if the file exists
fd -e d.ts "traffic-boost-settings"
# If found, examine its structure to ensure it exports the TrafficBoostSettings type
rg -A 5 "export.*interface.*TrafficBoostSettings"
Length of output: 424
src/rest-api/settings/class-settings-controller.php (1)
43-43: LGTM! The endpoint addition follows the established pattern.
The new Traffic Boost settings endpoint is properly integrated into the existing infrastructure, maintaining consistency with other endpoints.
src/content-helper/common/utils/permissions.ts (2)
7-10: LGTM! Interface changes are well-structured.
The interface properties are properly typed and organized. The addition of the TrafficBoost property aligns with the PR's objective of establishing Traffic Boost settings infrastructure.
22-25: LGTM! Default permissions are securely implemented.
The default values are properly initialized to false, providing secure defaults. The error handling ensures robustness when parsing permissions.
src/rest-api/settings/class-endpoint-traffic-boost-settings.php (3)
1-12: LGTM: File-level declarations follow WordPress standards
The file includes proper documentation, version information, strict typing, and namespace declaration.
39-41: Verify meta key naming convention
Let's confirm the meta key follows the established pattern used by other settings endpoints.
#!/bin/bash
# Search for other meta keys in the codebase
rg -g '*.php' "get_meta_key.*return.*'parsely_.*'" src/rest-api/28-30: Verify endpoint naming convention
Let's ensure the endpoint name 'traffic-boost' follows the naming convention of other endpoints in the codebase.
src/content-helper/dashboard-page/pages/dashboard/page-component.tsx (1)
4-4: Verify required components and types exist in the codebase.
Let's ensure all required components and types are properly defined.
src/UI/class-dashboard-page.php (2)
16-16: LGTM: Import statement is correctly placed.
The addition of the WP_REST_Request import follows WordPress coding standards and is properly used in the inject_traffic_boost_settings method.
160-163: LGTM: Script injection follows correct order.
The injection of permissions and settings is properly placed after the main script enqueue, following WordPress best practices for script dependencies.
src/content-helper/dashboard-page/pages/dashboard/page-component.tsx
Outdated
Show resolved
Hide resolved
|
The code looks good to me! I do have a more conceptual question though: instead of calling it The way I see it, Traffic Boost is a feature that lives inside the Parse.ly Dashboard (or whatever we're calling the SPA we're building, I'm still not sure Dashboard is the best name, since effectively the Dashboard will only be the first page). Comparing, for example, with the Editor Sidebar, which has its own settings namespace, and the individual features are part of that namespace - title suggestions, smart linking, etc. I think that making the settings namespace more broad, instead of pinning it to the Traffic Boost, will allow for easier expansion in the future, if/when we add more features to the so called "Dashboard". |
|
@vaurdan, this is a fair point. The difficulty of the moment is that we don't know how our first iteration of this will end up. We might very well finish with a single page that only needs TB settings. If the only settings we're having are TB related, then leaving it as is would be fine. If we're working with a broader range of settings, then I'd go with the general name. I think that sooner or later we'll get to what you suggest though. As things stand, for me both options are fine. Do you think it's better to go with the broader name right away? I think we'll need some more code for that. |
|
I do think we might end up with settings on the dashboard main page sooner than later - filtering options, for example - so IMO it makes sense to make it broader now and save the trouble later. |
Description
This PR introduces a Traffic Boost settings infrastructure (to be populated in the future with real settings), passing the setting values into our new dashboard page along with the current user's PCH permissions.
Motivation and context
Being able to pass settings and permissions into the dashboard page to tailor the experience accordingly.
How has this been tested?
Manual local testing.
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Documentation