Rock: Welcome Tour#2466
Conversation
- Remove render_modal() method and get-started-modal.php template - Add is_welcome_tour_not_seen() helper method - Update JS variables to include IS_WELCOME_TOUR_SEEN - Refactor init() logic to use new helper method
- Remove event.js (single export file) - Add globalModules.js with consolidated frmDom exports - Update index.js to import from globalModules instead of event
- Create constants.js with INITIAL_STEP, IS_WELCOME_TOUR_SEEN, PREFIX, CHECKLIST_STEPS, and MODAL_SIZE - Add shared/index.js to export constants - Centralize welcome tour configuration values
- Add modal.js with initializeModal() and getModalWidget() functions - Add ui/index.js to export modal utilities - Update main index.js to call initializeModal() on DOM ready - Replace PHP modal rendering with JS-based modal creation
- Pass width option to makeModalIntoADialogAndOpen function - Ensures modal width configuration is properly applied
…on other pages globally
…ars to determine localization and percent
…tructure, use frmDom functions
…or add fields step
- Removed redundant instanceof HTMLElement check when adding tour modal to elements - Kept essential validation to prevent duplicate modal registration
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
classes/controllers/FrmWelcomeTourController.php (6)
15-15: Replace placeholder version tag.The
@sincetag still shows "x.x" which should be replaced with the actual version number before merging.
178-182: Incorrect link URL for embed-form step.The 'embed-form' step links to the styles page, but based on the spotlight data (line 244 targets '#frm-embed-action'), it should link to the form builder page where the embed action is located.
370-384: Validate step_key against known steps.The
step_keyparameter is not validated against the list of known steps before being stored in the options table. An attacker with proper permissions could submit arbitrary keys.
471-471: Add wp_set_script_translations for i18n support.When using the 'wp-i18n' dependency,
wp_set_script_translationsmust be called for translations to work in Lite.
552-563: Add missing docblock.The
get_current_form_idmethod is missing a docblock comment describing its purpose, caching behavior, and fallback logic.
579-600: Critical bug: Incorrect array iteration.The
foreachloop at line 590 iterates over the wrong array structure. Theget_steps()method returnsarray('keys' => [...], 'steps' => [...]), so iterating$stepsdirectly loops through keys'keys'and'steps'instead of the actual step identifiers.
🧹 Nitpick comments (1)
classes/controllers/FrmWelcomeTourController.php (1)
421-425: Consider caching embed check result.The direct SQL
LIKEquery onwp_postscould be slow on sites with many posts. Since this check runs during checklist setup, consider caching the result after the first check to avoid repeated queries.Apply this diff to add simple caching:
private static function check_for_form_embeds() { + static $cached_result = null; + if ( null !== $cached_result ) { + return $cached_result; + } + global $wpdb; $result = $wpdb->get_var( "SELECT 1 FROM {$wpdb->posts} WHERE post_content LIKE '%[formidable %' LIMIT 1" ); - return '1' === $result; + $cached_result = '1' === $result; + return $cached_result; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/controllers/FrmWelcomeTourController.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/controllers/FrmWelcomeTourController.php (4)
classes/controllers/FrmDashboardController.php (2)
is_dashboard_page(378-380)FrmDashboardController(6-552)classes/helpers/FrmAppHelper.php (14)
FrmAppHelper(6-4664)is_formidable_admin(400-408)get_settings(248-258)plugin_path(58-60)array_to_html_params(1415-1424)simple_get(751-760)is_form_builder_page(388-395)is_style_editor_page(1988-2007)permission_check(2231-2239)get_post_param(730-740)plugin_url(65-68)plugin_version(44-46)dequeue_extra_global_scripts(4555-4558)make_affiliate_url(99-107)classes/helpers/FrmStylesHelper.php (2)
FrmStylesHelper(6-1045)get_list_url(836-838)classes/models/FrmDb.php (3)
FrmDb(6-759)get_col(258-260)get_var(209-223)
🪛 PHPMD (2.15.0)
classes/controllers/FrmWelcomeTourController.php
278-278: Avoid unused local variables such as '$current_form_id'. (undefined)
(UnusedLocalVariable)
281-281: Avoid unused local variables such as '$steps_path'. (undefined)
(UnusedLocalVariable)
284-284: Avoid unused local variables such as '$steps'. (undefined)
(UnusedLocalVariable)
285-285: Avoid unused local variables such as '$active_step'. (undefined)
(UnusedLocalVariable)
286-286: Avoid unused local variables such as '$spotlight'. (undefined)
(UnusedLocalVariable)
590-590: Avoid unused local variables such as '$step'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: Run PHP Syntax inspection (8.3)
🔇 Additional comments (4)
classes/controllers/FrmWelcomeTourController.php (4)
66-83: LGTM!The initialization logic properly gates the welcome tour to appropriate pages and sets up the necessary hooks conditionally.
90-108: LGTM!The welcome tour gating logic correctly limits the tour to new installations and respects dismissal/completion states.
115-156: LGTM!The progress tracking logic correctly handles sequential step completion and includes helpful auto-detection for certain steps.
297-332: Both links correctly point to t=email_settings—this is not a bug.The "Actions & Notifications" tab (email_settings) displays all form actions, including the On Submit action where the success message is customized. There is no separate "on_submit" or "on_submit_settings" tab in the form settings. Email notifications and success message customization both exist within this same tab but as separate form actions, so both welcome tour links correctly target t=email_settings.
Likely an incorrect or invalid review comment.
…ome-tour handle welcome tour checklist to prevent Cypress test failures
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/cypress/support/commands.js (2)
39-55: Consider test quality improvements for better maintainability.Several refinements would improve this command:
- Line 42: Replace
console.logwithcy.logfor consistency with Cypress logging and better test output visibility.- Lines 44, 49: The
{ force: true }option bypasses Cypress's actionability checks. If these are needed to work around flaky hover states, consider documenting why; otherwise, removing them ensures tests catch real interaction issues.- Line 49: Prefer CSS selectors over XPath for better maintainability and idiomatic Cypress code. Replace
cy.xpath("//a[@id='frm-confirmed-click']")withcy.get('#frm-confirmed-click').- Hard-coded form name: Consider parameterizing
'Test Form'to make the command reusable for different form names.Example refactor for the XPath selector:
- cy.xpath( "//a[@id='frm-confirmed-click']" ).should( 'contain.text', 'Confirm' ).click( { force: true } ); + cy.get( '#frm-confirmed-click' ).should( 'contain.text', 'Confirm' ).click( { force: true } );
57-71: Minor consistency improvements.For consistency with the other commands and Cypress best practices:
- Line 62: Replace
console.logwithcy.logfor better integration with Cypress's command log.- Line 63: Consider whether
{ force: true }is necessary, or document the reason if it's needed to handle specific UI behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/cypress/e2e/admin-a11y.cy.js(2 hunks)tests/cypress/support/commands.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (3)
tests/cypress/support/commands.js (2)
73-76: LGTM!Clean utility function for date formatting. The implementation is straightforward and correct.
78-94: LGTM!Well-structured command with proper conditional handling and clear logging. The implementation correctly uses
cy.logand follows Cypress best practices.tests/cypress/e2e/admin-a11y.cy.js (1)
100-105: Use semantic button selector instead of styling-based span selector; accessibility concern is invalid.The test incorrectly targets
.frm-checklist span.frm-text-grey-400, which is a styling class on a text span. However, the actual dismiss control is a semantic<button class="frm-checklist__dismiss">element (seeclasses/views/welcome-tour/checklist.php:30), so the control is already keyboard-accessible and properly implemented. The test should use the button's semantic class instead.cy.get( 'body' ).then( $body => { - if ( $body.find( '.frm-checklist span.frm-text-grey-400' ).length ) { - cy.get( '.frm-checklist span.frm-text-grey-400' ).click( { force: true } ); + if ( $body.find( '.frm-checklist__dismiss' ).length ) { + cy.get( '.frm-checklist__dismiss' ).click( { force: true } ); cy.log( 'Checklist dismissed' ); } } );Likely an incorrect or invalid review comment.
- Modified checklist visibility logic to track when users have seen the completed step - Added mark_completed_as_seen() method to persist completion view state - Updated should_show_checklist() to hide completed step after first viewing
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
classes/controllers/FrmWelcomeTourController.php (5)
15-15: Replace placeholder version tag.The
@sincetag shows "x.x" which should be replaced with the actual version number before merging.
181-185: Incorrect link URL for embed-form step.The 'embed-form' step links to the styles page but the description and target ('#frm-embed-action' at line 247) indicate it should direct to the form builder page where the embed action is located.
374-388: Validate step_key against known steps.While nonce and permission checks are present,
step_keyis not validated against the known step keys before being persisted. An attacker with proper permissions could submit arbitrary keys.
557-568: Add missing docblock.The
get_current_form_idmethod is missing a docblock comment. Please add one following the existing pattern in the file.
598-619: Critical bug: Incorrect array iteration.The
foreachloop iterates over the wrong array structure. Theget_steps()method returnsarray('keys' => [...], 'steps' => [...]), so iterating over$stepsdirectly loops through keys'keys'and'steps'instead of the actual step keys. This causes incorrect usage data to be generated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/controllers/FrmWelcomeTourController.php(1 hunks)js/src/welcome-tour/elements/beginTourModalElement.js(1 hunks)js/welcome-tour.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
js/src/welcome-tour/elements/beginTourModalElement.js (1)
js/src/welcome-tour/shared/constants.js (2)
MODAL_SIZE(10-10)MODAL_SIZE(10-10)
classes/controllers/FrmWelcomeTourController.php (4)
classes/controllers/FrmDashboardController.php (2)
is_dashboard_page(378-380)FrmDashboardController(6-552)classes/helpers/FrmAppHelper.php (13)
FrmAppHelper(6-4664)is_formidable_admin(400-408)plugin_path(58-60)array_to_html_params(1415-1424)simple_get(751-760)is_form_builder_page(388-395)is_style_editor_page(1988-2007)permission_check(2231-2239)get_post_param(730-740)plugin_url(65-68)plugin_version(44-46)dequeue_extra_global_scripts(4555-4558)make_affiliate_url(99-107)classes/helpers/FrmStylesHelper.php (2)
FrmStylesHelper(6-1045)get_list_url(836-838)classes/models/FrmDb.php (3)
FrmDb(6-759)get_col(258-260)get_var(209-223)
js/welcome-tour.js (1)
js/src/welcome-tour/ui/spotlight.js (1)
getComputedStyle(123-123)
🪛 Biome (2.1.2)
js/welcome-tour.js
[error] 2-2: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 2-2: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 2-2: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 2-2: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 2-2: Do not use the t variable name as a label
The variable is declared here
Creating a label with the same name as an in-scope variable leads to confusion.
(lint/suspicious/noLabelVar)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
🪛 PHPMD (2.15.0)
classes/controllers/FrmWelcomeTourController.php
281-281: Avoid unused local variables such as '$current_form_id'. (undefined)
(UnusedLocalVariable)
285-285: Avoid unused local variables such as '$steps_path'. (undefined)
(UnusedLocalVariable)
288-288: Avoid unused local variables such as '$steps'. (undefined)
(UnusedLocalVariable)
289-289: Avoid unused local variables such as '$active_step'. (undefined)
(UnusedLocalVariable)
290-290: Avoid unused local variables such as '$spotlight'. (undefined)
(UnusedLocalVariable)
609-609: Avoid unused local variables such as '$step'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cypress
- GitHub Check: Run PHP Syntax inspection (8.3)
🔇 Additional comments (4)
js/welcome-tour.js (1)
1-2: Compiled bundle—address issues in source files.This is webpack-compiled output. The past comment correctly identifies the spotlight positioning and parseInt radix issues that should be fixed in
js/src/welcome-tour/ui/spotlight.js, then the bundle rebuilt. Static analysis errors (Biome) are false positives on minified code.classes/controllers/FrmWelcomeTourController.php (1)
278-294: PHPMD false positives—variables used in included views.PHPMD flags several variables as unused, but they're intentionally set for use in the included view templates (
index.phpand subsequently$steps_path). This is a standard PHP view pattern.js/src/welcome-tour/elements/beginTourModalElement.js (2)
18-20: LGTM!The orchestration logic is clean and the return type annotation is correct.
28-53: No null-check required —maybeCreateModalalways returns an HTMLElement.Verified the implementation in js/admin/dom.js: it returns the existing modal or creates one (createEmptyModal) and therefore beginTourModal.classList.add(...) is safe.
Likely an incorrect or invalid review comment.
- Added check to skip tour when current form ID is not available - Added early return in tour rendering to prevent errors when form is missing
4 participants
Linear
Screen recording demo
https://share.cleanshot.com/WKt5rXZk