Feature/gh265 improve onboard flow#273
Conversation
WalkthroughCentralizes setup-wizard and onboarding state into the Settings API (new keys, getters/setters, onboarding status and date); updates Setup_Wizard to use Settings for step/state persistence, adds onboarding trigger and body-class helper, and adjusts step handling; activation now sets onboarding pending; Settings::clear_all_options expanded and tests updated; wizard templates simplified (several UI fields removed or replaced with hidden inputs) and onboarding CSS added; new Dashboard_Statistics class and onboarding dashboard templates wired into dashboard rendering; Scan_Posts_Event gained force_add_to_action_scheduler() with a new test. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Dashboard/Setup_Wizard.php (2)
257-288: Previous‑step handler should not call step submission handlers
handle_previous_step()currently invokes the step submission handlers even when the user is going back:// Upadate the current step to previous for rendering. if ( 'step-3' === $current_step ) { $this->handle_step_3(); } elseif ( 'step-2' === $current_step ) { $this->handle_step_2(); } ... Settings::update_setup_wizard_step( $previous_step );Problems:
handle_step_2()andhandle_step_3()expectiawmlf-next-stepand will add an error notice (“Next step is not set”) when navigating backwards.- They also re‑run side‑effects (updating options, marking onboarding complete, etc.) on a “Previous” click, which is unexpected.
You can simplify
handle_previous_step()to only move the wizard state back:- // Upadate the current step to previous for rendering. - if ( 'step-3' === $current_step ) { - $this->handle_step_3(); - } elseif ( 'step-2' === $current_step ) { - $this->handle_step_2(); - } + // Update the current step to the previous one for rendering only.The subsequent logic that computes
$previous_stepand callsSettings::update_setup_wizard_step( $previous_step );is already sufficient.
304-337: Correct the hidden field's actual value and confirm the fixThe hidden field doesn't have a literal
"0|1"value—it's actually rendered dynamically as either"1"or"0"viaSettings::is_link_processing_enabled( true ). However, the underlying issue is valid:isset()only checks presence, not the value, so it always returnstrueon submission, ignoring any disabled state from the previous page visit.The suggested fix is correct and will properly respect both enabled and disabled states:
$is_active = isset( $_POST['iawmlf_wizard_activate_link_fixer'] ) && '1' === sanitize_text_field( wp_unslash( $_POST['iawmlf_wizard_activate_link_fixer'] ) );This ensures that
PROCESS_LINKSreflects the user's actual choice, not a forcedtruewhenever post types are selected.
🧹 Nitpick comments (5)
templates/admin/wizard/step-2.php (3)
22-22: Escape hidden input value and double‑check the defaultThe hidden
iawmlf_wizard_activate_link_fixerfield currently prints the raw'1'/'0'value. It’s low risk, but you can harden it and make the intent clearer:-<input type="hidden" name="iawmlf_wizard_activate_link_fixer" value="<?php echo Settings::is_link_processing_enabled( true ) ? '1' : '0'; ?>" /> +<input type="hidden" name="iawmlf_wizard_activate_link_fixer" value="<?php echo esc_attr( Settings::is_link_processing_enabled( true ) ? '1' : '0' ); ?>" />Also worth confirming that
is_link_processing_enabled( true )is meant to default to “enabled” on a fresh install; if you want the wizard to start from “off”, passfalseinstead.
28-36: Clean up “??” prefixes in user‑facing copyBoth the intro and description strings start with literal
"??", which will look like placeholder/debug markers in the UI:esc_html_e( '?? You can set the Link Fixer...', ... ); esc_html_e( '?? Select the post types...', ... );If this isn’t intentional branding, please drop the prefix before release.
32-47: Improve label association and grouping for post‑type checkboxesThe label uses
for="iawmlf_wizard_post_types", but there is no element with thatid; only the checkboxes exist. For better semantics/a11y, consider:
- Wrapping the checkboxes in a
<fieldset>with a<legend>instead of a free‑floating<label>, or- Giving the checkbox group an explicit
idand updatingforto match.Not a blocker, but a small quality improvement.
src/Dashboard/Setup_Wizard.php (1)
70-82: add_onboarding_body_class: scope and sanitizationThe body‑class helper is straightforward, but two small considerations:
- It triggers on any admin page with
?iawmlf_onboarding=…, not just the wizard. That’s probably fine if your CSS is robust, but worth being aware of.- Since only the presence of the query arg matters, you might prefer
isset( $_GET['iawmlf_onboarding'] )for consistency withmaybe_trigger_onboarding_wizard().No functional blockers here.
src/Settings/Settings.php (1)
450-532: Alignget_onboarding_status()docblock and consider legacy value mappingThe onboarding helpers are a good abstraction, but there are two small issues:
- Docblock vs implementation mismatch
Docblock says:
* @param string $default_value Optional default value if not set. Default ONBOARDING_PENDING_OPTION.But the signature uses
self::ONBOARDING_COMPLETED_OPTIONas the default:public static function get_onboarding_status( string $default_value = self::ONBOARDING_COMPLETED_OPTION ): string {Either the doc or the default should be updated so they agree (and accurately reflect the intended default for “no stored value” cases).
- Potential backwards‑compat with legacy option values
get_onboarding_status()only recognizes the two new string constants as valid:$state = get_option( self::POST_ACTIVATION_ONBOARDING_KEY, $default_value ); return in_array( $state, array( self::ONBOARDING_COMPLETED_OPTION, self::ONBOARDING_PENDING_OPTION ), true ) ? $state : self::ONBOARDING_PENDING_OPTION;If any previous plugin versions ever stored boolean/
0/1or alternative strings underPOST_ACTIVATION_ONBOARDING_KEY, those installs will now seeONBOARDING_PENDING_OPTIONand be treated as “needs onboarding” again.If that’s a realistic history for this option, consider a small compatibility shim, e.g.:
if ( ! in_array( $state, array( self::ONBOARDING_COMPLETED_OPTION, self::ONBOARDING_PENDING_OPTION ), true ) ) { // Legacy mapping: truthy -> completed, falsy -> pending. if ( in_array( $state, array( '1', 1, true ), true ) ) { $state = self::ONBOARDING_COMPLETED_OPTION; } else { $state = self::ONBOARDING_PENDING_OPTION; } }If you’re sure earlier versions only ever used the new string values, then just fixing the docblock is enough.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.distignore(0 hunks)assets/css/src/admin/_wizard.scss(1 hunks)functions.php(1 hunks)src/Dashboard/Settings_Page.php(2 hunks)src/Dashboard/Setup_Wizard.php(10 hunks)src/Settings/Settings.php(4 hunks)src/Util/Esc.php(1 hunks)templates/admin/wizard/footer.php(1 hunks)templates/admin/wizard/step-1.php(1 hunks)templates/admin/wizard/step-2.php(2 hunks)templates/admin/wizard/step-3.php(1 hunks)tests/Settings/Test_Settings.php(2 hunks)tests/bootstrap.php(1 hunks)
💤 Files with no reviewable changes (1)
- .distignore
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T23:43:45.310Z
Learnt from: gin0115
Repo: a8cteam51/internet-archive-wayback-machine-link-fixer PR: 250
File: src/Settings/Settings.php:301-311
Timestamp: 2025-11-17T23:43:45.310Z
Learning: In the Internet Archive Wayback Machine Link Fixer plugin (file: src/Settings/Settings.php), the `iawmlf_add_own_content_to_wayback_machine` filter is intentionally applied after the production environment check in the `add_own_links()` method. This prevents the filter from overriding the hard block on archiving in non-production environments (staging, development, local), which is expected behavior to ensure staging/dev sites are never archived.
Applied to files:
templates/admin/wizard/step-3.php
🧬 Code graph analysis (7)
tests/bootstrap.php (1)
src/Settings/Settings.php (1)
Settings(22-564)
templates/admin/wizard/step-3.php (1)
src/Settings/Settings.php (2)
Settings(22-564)add_own_links(315-327)
templates/admin/wizard/step-2.php (1)
src/Settings/Settings.php (2)
Settings(22-564)is_link_processing_enabled(89-91)
tests/Settings/Test_Settings.php (1)
src/Settings/Settings.php (1)
Settings(22-564)
src/Dashboard/Setup_Wizard.php (2)
src/Settings/Settings.php (7)
Settings(22-564)is_wizard_completed(483-485)get_onboarding_status(527-532)update_setup_wizard_step(472-474)set_onboarding_status(509-516)set_wizard_completed(496-498)get_setup_wizard_step(459-461)src/Util/Environmental.php (2)
Environmental(20-38)is_production(27-37)
src/Dashboard/Settings_Page.php (1)
src/Settings/Settings.php (2)
Settings(22-564)is_wizard_completed(483-485)
functions.php (2)
src/Migration/Migrations.php (2)
Migrations(20-79)up(39-57)src/Settings/Settings.php (4)
Settings(22-564)get_onboarding_status(527-532)is_wizard_completed(483-485)set_onboarding_status(509-516)
🔇 Additional comments (15)
src/Util/Esc.php (1)
41-46: LGTM! Enabling disabled state for wizard buttons.The addition of the
disabledattribute to the button's allowed tags correctly supports the new wizard navigation behavior where certain buttons (e.g., the Previous button on the first step) need to be disabled. The implementation follows the proper wp_kses pattern for boolean attributes.assets/css/src/admin/_wizard.scss (1)
1-14: LGTM! Onboarding overlay styling is well-implemented.The CSS correctly creates a full-screen overlay for onboarding mode with proper positioning, z-index, and centered content layout.
tests/bootstrap.php (1)
68-70: LGTM! Test setup correctly bypasses onboarding.The test bootstrap appropriately marks onboarding as completed to prevent interference with test execution. This aligns with the new Settings-based onboarding state management.
src/Dashboard/Settings_Page.php (2)
100-100: LGTM! Wizard state check correctly migrated to Settings API.The change from
Setup_Wizard::is_setup_complete()toSettings::is_wizard_completed()aligns with the centralized state management approach introduced in this PR.
172-172: LGTM! Consistent migration to Settings-based wizard status.This change properly uses the centralized Settings API for wizard completion checks, maintaining consistency across the codebase.
functions.php (1)
103-114: LGTM! Onboarding state management correctly integrated into activation.The activation hook now properly manages onboarding state:
- Runs migrations first
- Checks existing onboarding/wizard status to avoid overwriting
- Sets onboarding to pending for new installations
The logic correctly prevents re-triggering onboarding for users who have already completed it.
templates/admin/wizard/step-3.php (1)
25-25: Verify: Auto Archiver toggle removed from user control.The Auto Archiver enable/disable UI was replaced with a hidden input that reads the current state from settings. This means users can no longer toggle the Auto Archiver setting within the wizard—they can only configure which post types to archive.
Is this intentional? If users should be able to enable/disable Auto Archiver during setup, the toggle UI should be retained.
tests/Settings/Test_Settings.php (2)
300-304: LGTM! Test coverage expanded for new onboarding/wizard options.The test now properly validates that the new onboarding and wizard-related settings are cleared by
Settings::clear_all_options(), ensuring clean uninstall behavior.
325-329: LGTM! Assertions complete for new option cleanup.The assertions correctly verify that all new onboarding/wizard options are removed after calling
clear_all_options().templates/admin/wizard/footer.php (1)
32-34: Previous button correctly disabled on first step — verification complete.The
disabledattribute is already included inEsc::wizard_allowed_tags()for thebuttonelement (seesrc/Util/Esc.php), so the attribute will not be stripped bywp_kses(). The implementation is correct and requires no changes.src/Dashboard/Setup_Wizard.php (3)
295-297: Step‑1 handler is clean and aligned with centralized state
handle_step_1()simply advances the wizard state viaSettings::update_setup_wizard_step( 'step-2' );, which fits the new centralized settings approach and the simplified step‑1 UI.No issues spotted here.
353-366: Step‑3 handler looks consistent with Settings/onboarding modelThe step‑3 submission logic:
- Reads
iawmlf_wizard_activate_auto_archiverand allowed post types.- Updates
ALLOW_OWN_CONTENT_SUBMISSIONS,ALLOWED_OWN_CONTENT_POST_TYPES, andROUTINELY_UPDATE_WAYBACK_MACHINE.- Marks the wizard as complete and sets onboarding status to completed.
Given
add_own_links()already force‑disables own‑content submissions in non‑production viaEnvironmental::is_production(), this is consistent and safe. No changes needed here.
435-467: UsingSettings::get_setup_wizard_step()for step data is a nice centralization
get_step_data()now pulls the current step fromSettings::get_setup_wizard_step('step-1')and applies the same environment‑based skipping for step‑3 before computing template, next, and progress. This keeps wizard state in one place and avoids direct option access here.Looks good.
src/Settings/Settings.php (2)
29-45: Option and onboarding constants are clear and consistentIntroducing explicit keys for wizard/onboarding state and discrete status constants (
ONBOARDING_COMPLETED_OPTION,ONBOARDING_PENDING_OPTION) keeps the Settings API coherent and self‑documenting. No issues here.Also applies to: 58-61
541-563: Expandedclear_all_options()coverage looks goodAdding the onboarding and wizard keys (plus
MINIMUM_CHECKS_BEFORE_BROKENandLINK_CHECK_DURATION_IN_DAYS) toclear_all_options()ensures a true reset of plugin state:delete_option( self::POST_ACTIVATION_ONBOARDING_KEY ); delete_option( self::MINIMUM_CHECKS_BEFORE_BROKEN ); delete_option( self::LINK_CHECK_DURATION_IN_DAYS ); delete_option( self::SETUP_WIZARD_STEP_KEY ); delete_option( self::SETUP_WIZARD_COMPLETED_KEY );This is a solid cleanup improvement.
|
|
||
| <div class="iawmlf-wizard__content__header"> | ||
| <h2><?php esc_html_e( 'Step 1: Configure the Wayback Machine API', 'internet-archive-wayback-machine-link-fixer' ); ?></h2> | ||
| <h2><?php esc_html_e( '??Step 1: Configure the Wayback Machine API', 'internet-archive-wayback-machine-link-fixer' ); ?></h2> |
There was a problem hiding this comment.
Critical: Remove debugging placeholder "??" from heading.
The "??" prefix appears to be debugging or placeholder text that was accidentally left in the translatable string. This will be visible to users in the wizard UI.
Apply this diff:
- <h2><?php esc_html_e( '??Step 1: Configure the Wayback Machine API', 'internet-archive-wayback-machine-link-fixer' ); ?></h2>
+ <h2><?php esc_html_e( 'Step 1: Configure the Wayback Machine API', 'internet-archive-wayback-machine-link-fixer' ); ?></h2>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <h2><?php esc_html_e( '??Step 1: Configure the Wayback Machine API', 'internet-archive-wayback-machine-link-fixer' ); ?></h2> | |
| <h2><?php esc_html_e( 'Step 1: Configure the Wayback Machine API', 'internet-archive-wayback-machine-link-fixer' ); ?></h2> |
🤖 Prompt for AI Agents
In templates/admin/wizard/step-1.php around line 22, the heading contains a
debugging placeholder "??" inside the translatable string; remove the "??" so
the string reads normally (e.g. "Step 1: Configure the Wayback Machine API") and
ensure esc_html_e uses the corrected string to avoid showing the placeholder to
users and keep translation keys unchanged.
| <p><?php esc_html_e( '??To archive more than 4000 links from your site to the Wayback Machine per day, you\'ll need a free archive.org account. Once you have your account, enter the API Access Key and Secret Key below. (Optional)', 'internet-archive-wayback-machine-link-fixer' ); ?> | ||
| <br /><a href="https://archive.org/account/s3.php" target="_blank"><?php esc_html_e( 'Get your API keys here.', 'internet-archive-wayback-machine-link-fixer' ); ?></a></p> |
There was a problem hiding this comment.
Critical: Remove debugging placeholder "??" from intro text.
The "??" prefix appears to be debugging or placeholder text that was accidentally left in the translatable string. This will be visible to users in the wizard.
Apply this diff:
- <p><?php esc_html_e( '??To archive more than 4000 links from your site to the Wayback Machine per day, you\'ll need a free archive.org account. Once you have your account, enter the API Access Key and Secret Key below. (Optional)', 'internet-archive-wayback-machine-link-fixer' ); ?>
+ <p><?php esc_html_e( 'To archive more than 4000 links from your site to the Wayback Machine per day, you\'ll need a free archive.org account. Once you have your account, enter the API Access Key and Secret Key below. (Optional)', 'internet-archive-wayback-machine-link-fixer' ); ?>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <p><?php esc_html_e( '??To archive more than 4000 links from your site to the Wayback Machine per day, you\'ll need a free archive.org account. Once you have your account, enter the API Access Key and Secret Key below. (Optional)', 'internet-archive-wayback-machine-link-fixer' ); ?> | |
| <br /><a href="https://archive.org/account/s3.php" target="_blank"><?php esc_html_e( 'Get your API keys here.', 'internet-archive-wayback-machine-link-fixer' ); ?></a></p> | |
| <p><?php esc_html_e( 'To archive more than 4000 links from your site to the Wayback Machine per day, you\'ll need a free archive.org account. Once you have your account, enter the API Access Key and Secret Key below. (Optional)', 'internet-archive-wayback-machine-link-fixer' ); ?> | |
| <br /><a href="https://archive.org/account/s3.php" target="_blank"><?php esc_html_e( 'Get your API keys here.', 'internet-archive-wayback-machine-link-fixer' ); ?></a></p> |
🤖 Prompt for AI Agents
In templates/admin/wizard/step-1.php around lines 26 to 27, the translatable
paragraph includes a stray debugging placeholder "??" at the start of the
string; remove the "??" so the esc_html_e call begins with the proper
user-facing sentence, preserving the existing translation domain and HTML
structure (keep the br and anchor unchanged) so the visible text no longer shows
the debugging characters.
…allow link fixing, it will bump the first scan of own content to instant time with a priority of 0 to get the process going
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/Dashboard/Setup_Wizard.php (1)
113-135: Onboarding redirect lacks capability and AJAX protections (duplicate concern).This method redirects every admin request without checking:
- Whether the request is AJAX or cron (breaking background flows)
- Whether the user has the required capability (trapping non-admins in redirect loops)
The
manage_optionscheck at line 147 only protects the submenu registration, not this redirect which fires earlier.Add protections at the start of the method:
public function maybe_trigger_onboarding_wizard(): void { + if ( wp_doing_ajax() || ( defined( 'DOING_CRON' ) && DOING_CRON ) ) { + return; + } + + if ( ! current_user_can( 'manage_options' ) ) { + return; + } + if ( Settings::ONBOARDING_COMPLETED_OPTION === Settings::get_onboarding_status() || Settings::is_wizard_completed() ) {Based on learnings (past review comment).
🧹 Nitpick comments (3)
tests/Event/Test_Scan_Posts_Event.php (3)
37-38: Prefer Action Scheduler APIs over direct SQL queries.Direct SQL queries tightly couple the test to the Action Scheduler database schema. If the schema changes in a future version, these queries will break. Action Scheduler provides API functions to query scheduled actions.
Consider using Action Scheduler's API functions instead:
-// Get the time of the scheduled action. -$actions = $GLOBALS['wpdb']->get_results( "SELECT * FROM {$GLOBALS['wpdb']->prefix}actionscheduler_actions WHERE hook='iawmlf_scan_existing_posts' AND status='pending'" ); -$time_1 = $actions[0]->scheduled_date_gmt; +// Get the scheduled action using Action Scheduler API. +$time_1 = as_next_scheduled_action( 'iawmlf_scan_existing_posts', array(), 'iawmlf_event' );Similarly for the second query. This would make the test more maintainable and resilient to Action Scheduler updates.
Also applies to: 44-46
49-49: String comparison of timestamps may be fragile.Comparing datetime strings directly (line 49) assumes consistent formatting and can fail if timezone handling or storage format changes. Consider converting to Unix timestamps for more robust comparison.
If using
as_next_scheduled_action()as suggested above, the comparison becomes simpler since it returns Unix timestamps:-$this->assertTrue( $time_2 < $time_1, 'The scheduled time was not updated to an earlier time.' ); +$this->assertLessThan( $time_1, $time_2, 'The scheduled time was not updated to an earlier time.' );
29-51: Consider adding test cleanup to remove scheduled actions.The test schedules actions but doesn't clean them up in a tearDown method. While this may not affect subsequent tests if the database is reset between tests, explicit cleanup improves test isolation and makes the test's lifecycle clearer.
Add a tearDown method:
public function tearDown(): void { parent::tearDown(); // Clean up scheduled actions. if ( function_exists( 'as_unschedule_all_actions' ) ) { as_unschedule_all_actions( 'iawmlf_scan_existing_posts' ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Dashboard/Setup_Wizard.php(11 hunks)src/Event/Scan_Posts_Event.php(1 hunks)tests/Event/Test_Scan_Own_Posts_Event.php(1 hunks)tests/Event/Test_Scan_Posts_Event.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/Event/Test_Scan_Own_Posts_Event.php
🧰 Additional context used
🧬 Code graph analysis (3)
src/Event/Scan_Posts_Event.php (1)
src/Settings/Settings.php (2)
Settings(22-564)should_scan_existing_posts(210-216)
tests/Event/Test_Scan_Posts_Event.php (2)
src/Event/Scan_Posts_Event.php (3)
Scan_Posts_Event(24-174)add_to_action_scheduler(97-124)force_add_to_action_scheduler(72-90)src/Settings/Settings.php (1)
Settings(22-564)
src/Dashboard/Setup_Wizard.php (2)
src/Event/Scan_Posts_Event.php (2)
Scan_Posts_Event(24-174)force_add_to_action_scheduler(72-90)src/Settings/Settings.php (8)
Settings(22-564)is_wizard_completed(483-485)get_onboarding_status(527-532)update_setup_wizard_step(472-474)should_scan_existing_posts(210-216)set_onboarding_status(509-516)set_wizard_completed(496-498)get_setup_wizard_step(459-461)
🔇 Additional comments (8)
src/Event/Scan_Posts_Event.php (1)
65-90: No issues found withas_enqueue_async_action()call.The function signature is
as_enqueue_async_action( string $hook, array $args = array(), string $group = '', bool $unique = false, int $priority = 10 ), and the call on line 89 correctly passes all five arguments in the expected order with matching types:self::HANDLE(hook),array()(args),'iawmlf_event'(group),false(unique), and0(priority). The implementation is correct.src/Dashboard/Setup_Wizard.php (7)
15-15: LGTM!The import is necessary for the new scan triggering functionality in
handle_step_2().
26-32: LGTM!The private visibility for these constants is appropriate since they're only used internally within the
Setup_Wizardclass.
52-58: LGTM!Proper deprecation notice added, and the delegation to
Settings::is_wizard_completed()aligns with the centralized state management approach.
91-91: LGTM!The migration to
Settings::is_wizard_completed()is consistent with the centralized state management approach.
288-288: LGTM!The migration to
Settings::update_setup_wizard_step()is consistent with the centralized state management approach.Also applies to: 297-297
375-377: LGTM!The migration to Settings methods (
update_setup_wizard_step,set_wizard_completed,set_onboarding_status) is consistent with the centralized state management approach.
448-448: LGTM!The migration to
Settings::get_setup_wizard_step()is consistent with the centralized state management approach, and the default value is appropriate.
…s since install and we still have posts to process
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Dashboard/Dashboard_Notifications.php (1)
88-101: Fix array formatting to resolve PHPCS errors on dashboard widget dataThe new entries are fine functionally, but PHPCS is complaining about arrow alignment and the missing trailing comma on the last item in this multi‑line array.
- 'iawmlf_failed_check_count' => Settings::get_failed_count(), - 'iawmlf_onboarding_details' => Dashboard_Statistics::get_onboarding_statistics(), - 'iawmlf_link_stats' => Dashboard_Statistics::get_link_statistics() + 'iawmlf_failed_check_count' => Settings::get_failed_count(), + 'iawmlf_onboarding_details' => Dashboard_Statistics::get_onboarding_statistics(), + 'iawmlf_link_stats' => Dashboard_Statistics::get_link_statistics(),Optionally, you may also add a
use Internet_Archive\Wayback_Machine_Link_Fixer\Dashboard\Dashboard_Statistics;at the top for consistency with the other Dashboard classes, though it isn’t required for correctness.templates/admin/dashboard/page.php (1)
27-44: Use the pre‑normalized total link count when passing data to the onboarding templateYou already defensively normalize
total_linksinto$iawmlf_total_links_countusing?? 0, but the onboarding call still accesses$iawmlf_link_stats['total_links']directly. Using the normalized variable avoids any potential undefined‑index notices and keeps the logic DRY.- iawmlf_render_template( - 'admin/dashboard/onboarding.php', - array( - 'iawmlf_onboarding_details' => $iawmlf_onboarding_details, - 'iawmlf_total_links_count' => $iawmlf_link_stats['total_links'], - 'iawmlf_link_table' => \Internet_Archive\Wayback_Machine_Link_Fixer\Dashboard\Report_Page::get_page_url(), - ) - ); + iawmlf_render_template( + 'admin/dashboard/onboarding.php', + array( + 'iawmlf_onboarding_details' => $iawmlf_onboarding_details, + 'iawmlf_total_links_count' => $iawmlf_total_links_count, + 'iawmlf_link_table' => \Internet_Archive\Wayback_Machine_Link_Fixer\Dashboard\Report_Page::get_page_url(), + ) + );As a future clean‑up (no change needed for this PR), you might also consider updating the widget template to accept a numeric
$iawmlf_total_links_countinstead of constructing a dummy array viaarray_fill(), which would simplify the data flow.Also applies to: 112-129
src/Settings/Settings.php (1)
567-589: Ensureclear_all_options()also deletesONBOARDING_DATE_KEY
clear_all_options()has been expanded to remove several onboarding-related options, which is good:delete_option( self::POST_ACTIVATION_ONBOARDING_KEY ); delete_option( self::SETUP_WIZARD_STEP_KEY ); delete_option( self::SETUP_WIZARD_COMPLETED_KEY );However,
ONBOARDING_DATE_KEYis not deleted, so a previous onboarding date will linger after a “clear all options” call and may continue to influence onboarding statistics.Consider adding:
delete_option( self::SETUP_WIZARD_COMPLETED_KEY ); + delete_option( self::ONBOARDING_DATE_KEY );to keep cleanup symmetrical with the new onboarding-related state.
♻️ Duplicate comments (5)
src/Dashboard/Setup_Wizard.php (5)
71-83: Fix typo and static/instance mismatch inadd_onboarding_body_class()
- Docblock typo: "oboarding" → "onboarding".
- Method is declared
staticbut registered as an instance callback (array( $this, 'add_onboarding_body_class' )); either:
- Drop
staticon the method, or- Register it as
array( __CLASS__, 'add_onboarding_body_class' ).Functionally it works today, but tightening this avoids confusion.
113-143: Align onboarding redirect capability with Settings and ensure minimal scopeThe added AJAX/cron guards are good, but the capability check is hard-coded:
if ( ! current_user_can( 'manage_options' ) ) { return; }Elsewhere, capabilities are centralized via
Settings::get_reporting_page_capability(). For consistency and extensibility (e.g., sites that adjust capabilities via the filter), this should use that helper:- if ( ! current_user_can( 'manage_options' ) ) { + if ( ! current_user_can( Settings::get_reporting_page_capability() ) ) { return; }That keeps onboarding redirects aligned with whatever capability the plugin exposes for its settings/reporting pages.
266-297: “Previous” navigation still calls step handlers and will trigger validation errorsWhen handling a "Previous" click,
handle_previous_step()still invokes the step handlers:// Upadate the current step to previous for rendering. if ( 'step-3' === $current_step ) { $this->handle_step_3(); } elseif ( 'step-2' === $current_step ) { $this->handle_step_2(); }But
handle_step_2()andhandle_step_3()both expectiawmlf-next-stepin$_POSTand emit an error if it’s missing. On a "Previous" submit, onlyiawmlf-previous-stepis present, so you’ll get spurious “Next step is not set” notices instead of moving back a step.For backward navigation, you generally just need to adjust the stored step, not re-run forward validation. Consider removing these calls:
- // Upadate the current step to previous for rendering. - if ( 'step-3' === $current_step ) { - $this->handle_step_3(); - } elseif ( 'step-2' === $current_step ) { - $this->handle_step_2(); - } -The rest of
handle_previous_step()already computes and stores the prior step correctly.
326-370: Step‑2 overwrites user settings on rerun and mis-triggers scan schedulingSeveral issues in
handle_step_2():
- Unconditional overwrite of
FIXER_OPTIONon rerunupdate_option( Settings::FIXER_OPTION, Settings::FIXER_OPTION_REPLACE_LINK );This resets the fixer behavior every time the wizard is rerun, even if the user customized it elsewhere. Typically, you either:
- Only set a default the first time (when onboarding is pending), or
- Drive the value from a form field.
Example of “initial setup only” behavior:
- update_option( Settings::FIXER_OPTION, Settings::FIXER_OPTION_REPLACE_LINK ); + // Set default fixer option only during initial onboarding. + if ( Settings::ONBOARDING_PENDING_OPTION === Settings::get_onboarding_status() ) { + update_option( Settings::FIXER_OPTION, Settings::FIXER_OPTION_REPLACE_LINK ); + }
- Inverted onboarding check for
Scan_Posts_Event::force_add_to_action_scheduler()Comment says:
// If we are still in initial onboarding, force the scan own content to true.but the condition is:
if ( Settings::ONBOARDING_PENDING_OPTION !== Settings::get_onboarding_status() ) { Scan_Posts_Event::force_add_to_action_scheduler(); }This runs the scan when not in initial onboarding (i.e., most often on reruns), and skips it for first-time users. The condition should be equality:
- if ( Settings::ONBOARDING_PENDING_OPTION !== Settings::get_onboarding_status() ) { + if ( Settings::ONBOARDING_PENDING_OPTION === Settings::get_onboarding_status() ) { // Force the scan own content to trigger early. Scan_Posts_Event::force_add_to_action_scheduler(); }
- SCAN_EXISTING_POSTS recalculation is slightly opaque
update_option( Settings::SCAN_EXISTING_POSTS, Settings::should_scan_existing_posts( $process_links ) );Here,
$process_links(a boolean) is passed as the default toshould_scan_existing_posts(), which itself checks link processing and another option. It works, but the intent is harder to follow. Explicitly setting SCAN_EXISTING_POSTS based on$process_links(or a dedicated field) would be clearer.Overall, I’d recommend:
- Guarding FIXER_OPTION updates so reruns don’t clobber custom settings.
- Fixing the inverted onboarding condition for
Scan_Posts_Event.
377-403: Step‑3 unconditionally forces routine updates and may overwrite user preferencesIn
handle_step_3():update_option( Settings::ROUTINELY_UPDATE_WAYBACK_MACHINE, true );This hard-codes the “routinely update Wayback Machine” behavior to
trueevery time the wizard completes step‑3, even if the user previously disabled it and reruns the wizard later.As with the fixer option in step‑2, this is better treated as an initial default or driven by a form field. For minimal change:
- update_option( Settings::ROUTINELY_UPDATE_WAYBACK_MACHINE, true ); + // Set a default only if the option has never been configured. + if ( false === get_option( Settings::ROUTINELY_UPDATE_WAYBACK_MACHINE, false ) ) { + update_option( Settings::ROUTINELY_UPDATE_WAYBACK_MACHINE, true ); + }That avoids silently overriding existing user preferences on reruns while still providing a sensible default for first-time setups.
🧹 Nitpick comments (6)
README.md (1)
811-829: Replace hard tabs with spaces in new code examples to satisfy markdownlintmarkdownlint is flagging MD010 (hard tabs) on the
returnlines in the two new cache-expiry filter examples. Converting those to spaces will clear the warning and keep formatting consistent.-```php -add_filter( 'iawmlf_dashboard_link_stats_cache_expiry', function( int $cache_expiry ): int { - return 5 * \MINUTE_IN_SECONDS; // Cache for 5 minutes -}); -``` +```php +add_filter( 'iawmlf_dashboard_link_stats_cache_expiry', function( int $cache_expiry ): int { + return 5 * \MINUTE_IN_SECONDS; // Cache for 5 minutes +}); +``` @@ -```php -add_filter( 'iawmlf_dashboard_onboarding_stats_cache_expiry', function( int $cache_expiry ): int { - return 5 * \MINUTE_IN_SECONDS; // Cache for 5 minutes -}); -``` +```php +add_filter( 'iawmlf_dashboard_onboarding_stats_cache_expiry', function( int $cache_expiry ): int { + return 5 * \MINUTE_IN_SECONDS; // Cache for 5 minutes +}); +```templates/admin/dashboard/widget.php (1)
1-18: Update widget docblock to include onboarding and stats parametersThe template now relies on
$iawmlf_onboarding_detailsand$iawmlf_link_stats, but they aren’t documented in the file header. It’s worth adding them so consumers know the full expected context./** * Template for the dashboard widget. @@ - * @param int $iawmlf_link_check_duration Number of days between link checks. - * @param int $iawmlf_failed_check_count Number of failed checks before marking as broken. + * @param int $iawmlf_link_check_duration Number of days between link checks. + * @param int $iawmlf_failed_check_count Number of failed checks before marking as broken. + * @param array $iawmlf_onboarding_details Onboarding details and post counts. + * @param array $iawmlf_link_stats Link statistics array (e.g. total_links). */Also applies to: 32-43
src/Dashboard/Dashboard_Page.php (1)
73-81: Reuse computed$link_statsinstead of callingDashboard_Statistics::get_link_statistics()twiceThe new helpers and the switch to
Dashboard_Statisticslook good, but inrender_page()you already fetch$link_statsat Line 176 and then callDashboard_Statistics::get_link_statistics()again at Line 249 foriawmlf_link_stats.To avoid redundant work (and keep behavior consistent if the implementation of
get_link_statistics()changes), consider reusing the existing variable:- $link_stats = Dashboard_Statistics::get_link_statistics(); + $link_stats = Dashboard_Statistics::get_link_statistics(); ... - 'iawmlf_link_stats' => Dashboard_Statistics::get_link_statistics(), + 'iawmlf_link_stats' => $link_stats,You might also want to remove
STATS_TRANSIENT_KEYif nothing else uses it anymore.Also applies to: 176-183, 249-269
src/Dashboard/Dashboard_Statistics.php (2)
208-226: Onboarding stats aren’t actually cached; either cache them or drop the transient read
get_onboarding_statistics()reads fromONBOARDING_STATS_TRANSIENT_KEYbut never callsset_transient()after compiling fresh stats, so:
- The first call always falls back to
compile_onboarding_statistics()whenever the transient is missing or invalid.- Subsequent calls in the same request still bypass caching, since no transient is written.
If you intend onboarding stats to be cached similarly to link stats, you probably want:
public static function get_onboarding_statistics(): array { $from_cache = get_transient( self::ONBOARDING_STATS_TRANSIENT_KEY ); @@ - if ( false === $from_cache || ! is_array( $from_cache ) ) { - $stats = self::compile_onboarding_statistics(); - } else { + if ( false === $from_cache || ! is_array( $from_cache ) ) { + $stats = self::compile_onboarding_statistics(); + } else { @@ - } - - return $stats; + } + + // Optionally cache for reuse; adjust expiry as needed. + set_transient( self::ONBOARDING_STATS_TRANSIENT_KEY, $stats, 5 * \MINUTE_IN_SECONDS ); + + return $stats; }Alternatively, if you don’t want caching, you can simplify the method by removing the transient access entirely.
Also applies to: 273-302
273-302: Minor doc/comment mismatch in onboarding compile helper
compile_onboarding_statistics()returns a structured array withshow_onboardingset tofalsewhen onboarding is over, but the inline comment says:// If we have no oneboarding date or its 7 days or more ago, return empty array, as onboarding is over.Since you’re deliberately returning defaulted counts and metadata, consider updating the comment to match the actual behavior (e.g., “return defaults with show_onboarding = false”). This avoids confusion for future readers, but it’s not functionally problematic.
src/Settings/Settings.php (1)
451-558: Wizard and onboarding accessors centralize state cleanly (minor doc tweak suggested)The new helpers:
get_setup_wizard_step()/update_setup_wizard_step()is_wizard_completed()/set_wizard_completed()get_onboarding_date()/set_onboarding_date()set_onboarding_status()/get_onboarding_status()nicely encapsulate all wizard/onboarding state and add validation for onboarding statuses.
One small polish item: the docblock for
get_onboarding_status()says the default isONBOARDING_PENDING_OPTION, but the parameter default isONBOARDING_COMPLETED_OPTION:/** * @param string $default_value Optional default value if not set. Default ONBOARDING_PENDING_OPTION. */ public static function get_onboarding_status( string $default_value = self::ONBOARDING_COMPLETED_OPTION ): string {Either the comment or the default should be updated so they agree, to avoid confusion for future callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(1 hunks)src/Dashboard/Dashboard_Notifications.php(1 hunks)src/Dashboard/Dashboard_Page.php(4 hunks)src/Dashboard/Dashboard_Statistics.php(1 hunks)src/Dashboard/Setup_Wizard.php(10 hunks)src/Settings/Settings.php(4 hunks)templates/admin/dashboard/onboarding.php(1 hunks)templates/admin/dashboard/page.php(2 hunks)templates/admin/dashboard/widget.php(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T23:43:45.310Z
Learnt from: gin0115
Repo: a8cteam51/internet-archive-wayback-machine-link-fixer PR: 250
File: src/Settings/Settings.php:301-311
Timestamp: 2025-11-17T23:43:45.310Z
Learning: In the Internet Archive Wayback Machine Link Fixer plugin (file: src/Settings/Settings.php), the `iawmlf_add_own_content_to_wayback_machine` filter is intentionally applied after the production environment check in the `add_own_links()` method. This prevents the filter from overriding the hard block on archiving in non-production environments (staging, development, local), which is expected behavior to ensure staging/dev sites are never archived.
Applied to files:
src/Dashboard/Setup_Wizard.php
🧬 Code graph analysis (6)
src/Dashboard/Dashboard_Page.php (3)
src/Dashboard/Report_Page.php (1)
get_page_url(58-60)src/Dashboard/Settings_Page.php (1)
get_page_url(62-64)src/Dashboard/Dashboard_Statistics.php (3)
Dashboard_Statistics(22-339)get_link_statistics(41-61)get_onboarding_statistics(208-226)
src/Dashboard/Dashboard_Notifications.php (1)
src/Dashboard/Dashboard_Statistics.php (3)
Dashboard_Statistics(22-339)get_onboarding_statistics(208-226)get_link_statistics(41-61)
src/Dashboard/Dashboard_Statistics.php (3)
src/Link/Link.php (6)
Link(21-497)is_broken(180-182)get_id(118-120)has_archived_href(257-259)get_last_check(299-306)get_archive_process(372-374)src/Settings/Settings.php (3)
Settings(22-590)get_onboarding_date(508-511)get_allowed_post_types(101-103)src/Link/Link_Repository.php (2)
Link_Repository(23-673)query_links(394-510)
templates/admin/dashboard/page.php (2)
functions.php (1)
iawmlf_render_template(195-230)src/Dashboard/Report_Page.php (2)
Report_Page(23-361)get_page_url(58-60)
templates/admin/dashboard/widget.php (3)
src/Dashboard/Dashboard_Page.php (3)
Dashboard_Page(30-294)is_current_page(78-81)get_page_url(88-90)functions.php (1)
iawmlf_render_template(195-230)src/Dashboard/Report_Page.php (2)
Report_Page(23-361)get_page_url(58-60)
src/Dashboard/Setup_Wizard.php (3)
src/Event/Scan_Posts_Event.php (2)
Scan_Posts_Event(24-174)force_add_to_action_scheduler(72-90)src/Settings/Settings.php (10)
Settings(22-590)is_wizard_completed(484-486)get_onboarding_status(553-558)update_setup_wizard_step(473-475)set_onboarding_status(535-542)get_onboarding_date(508-511)set_onboarding_date(522-524)should_scan_existing_posts(211-217)set_wizard_completed(497-499)get_setup_wizard_step(460-462)src/Util/Environmental.php (2)
Environmental(20-38)is_production(27-37)
🪛 GitHub Actions: PHP Coding Standards
src/Dashboard/Dashboard_Notifications.php
[warning] 100-100: Array double arrow not aligned correctly; expected 14 space(s) between "'iawmlf_link_stats'" and double arrow, but found 9. (WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned)
[error] 100-100: There should be a comma after the last array item in a multi-line array. (NormalizedArrays.Arrays.CommaAfterLast.MissingMultiLine)
src/Dashboard/Dashboard_Statistics.php
[error] 211-211: Expected 1 spaces after opening parenthesis; 0 found (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
[error] 211-211: Expected 1 spaces before closing parenthesis; 0 found (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
templates/admin/dashboard/widget.php
[warning] 23-23: This comment is 56% valid code; is this commented out code? (Squiz.PHP.CommentedOutCode.Found)
🪛 markdownlint-cli2 (0.18.1)
README.md
817-817: Hard tabs
Column: 1
(MD010, no-hard-tabs)
827-827: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ 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). (1)
- GitHub Check: build (ubuntu-latest, 8.4)
🔇 Additional comments (5)
src/Dashboard/Setup_Wizard.php (3)
300-310: Onboarding completion helper is straightforward and consistent
complete_onboarding()centralizes updating the onboarding status and date, and the “only set date if missing” behavior makes repeated calls idempotent.No issues here; the helper looks solid and keeps the rest of the wizard code cleaner.
317-319: Step‑1 handler is minimal but correct
handle_step_1()now only advances the wizard tostep-2viaSettings::update_setup_wizard_step( 'step-2' );. For a purely informational first step, that’s sufficient and keeps logic simple.No changes needed.
471-503: Settings‑backed wizard state makes step resolution more robustSwitching
get_step_data()to useSettings::get_setup_wizard_step()and centralizing step validation and skipping ofstep-3for non‑production environments looks good.The logic for:
- Normalizing invalid state back to
'step-1',- Removing
step-3when not production, and- Computing
nextandprogressfrom theSTEPSarrayis clear and matches the intended environment-dependent flow.
No changes needed here.
src/Dashboard/Dashboard_Statistics.php (1)
41-61: Link statistics implementation and caching look solidThe link stats pipeline:
get_link_statistics()→ transient-backed with configurable expiry viaiawmlf_dashboard_link_stats_cache_expiry.compile_link_statistics()computes all the necessary counts and recent checks.normalize_link_statistics()ensures the cached payload remains well-formed.The overall design is good and keeps dashboard rendering lightweight after the first hit. One minor polish point: the docblock for
get_link_statistics()doesn’t mention thelast_checksfield, which is part of the returned array and used elsewhere; updating the annotation would help static analysis but isn’t functionally blocking.Also applies to: 123-195
src/Settings/Settings.php (1)
29-46: New settings constants for wizard/onboarding are well-structuredThe additional option keys (
POST_ACTIVATION_ONBOARDING_KEY,SETUP_WIZARD_STEP_KEY,SETUP_WIZARD_COMPLETED_KEY,ONBOARDING_DATE_KEY) and onboarding status constants (ONBOARDING_COMPLETED_OPTION,ONBOARDING_PENDING_OPTION) are named consistently with the existing scheme and keep the settings surface understandable.No issues here.
Also applies to: 59-62
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Event/Scan_Posts_Event.php (1)
72-90: Reuse the stricter Action Scheduler guard for the normal scheduler path as wellThe new
force_add_to_action_scheduler()correctly checks foras_unschedule_all_actions()andas_enqueue_async_action()before use.add_to_action_scheduler()still only checksas_has_scheduled_action()but later callsas_enqueue_async_action()/as_schedule_single_action(), which could fatal if Action Scheduler is only partially loaded. Consider centralizing a guard that verifies all required functions and reusing it in both methods to keep error handling consistent.
🧹 Nitpick comments (8)
templates/admin/dashboard/widget.php (2)
26-45: Guard against missing onboarding and stats keys in template contextThe onboarding block assumes
$iawmlf_onboarding_details['show_onboarding']and$iawmlf_link_stats['total_links']always exist. If this widget is ever rendered without those keys, PHP notices will be raised. Consider tightening the condition and defaults, e.g.:<?php if ( ! empty( $iawmlf_onboarding_details['show_onboarding'] ) && ! Dashboard_Page::is_current_page() ) : ?> … 'iawmlf_total_links_count' => isset( $iawmlf_link_stats['total_links'] ) ? (int) $iawmlf_link_stats['total_links'] : 0,
7-18: Update template docblock to reflect new data inputsThe docblock lists many
$iawmlf_*parameters but omits$iawmlf_onboarding_detailsand$iawmlf_link_stats, which are now required by the onboarding section. Add these to the param list (with their expected shapes) so callers and static analysis have an accurate contract.Also applies to: 31-40
src/Dashboard/Setup_Wizard.php (2)
71-83: Alignadd_onboarding_body_classstatic-ness with how it’s hooked
add_onboarding_body_class()is declaredpublic staticbut is registered as an instance callback (array( $this, 'add_onboarding_body_class' )). This works in PHP, but it’s inconsistent and can trip linters. Either dropstaticon the method or register it asarray( __CLASS__, 'add_onboarding_body_class' )to keep declaration and usage aligned.Also applies to: 45-47
113-143: Consider reusing Settings capability helper for onboarding redirect
maybe_trigger_onboarding_wizard()currently hard-codescurrent_user_can( 'manage_options' ). Elsewhere, capabilities are centralized viaSettings::get_reporting_page_capability(). If you want integrators to be able to relax/tighten access through one filter, it may be cleaner to use that helper here as well for consistency (unless the wizard is intentionally restricted tomanage_optionsonly).src/Dashboard/Dashboard_Statistics.php (2)
314-341: Optimizeget_post_count()query to reduce overhead
get_post_count()runs aWP_Queryover all allowed post types without specifyingfields/posts_per_page, even though onlyfound_postsis used. To reduce memory and query cost you can set:$args = array( 'post_type' => Settings::get_allowed_post_types(), 'posts_per_page' => 1, 'fields' => 'ids', 'no_found_rows' => false, // still need found_posts 'cache_results' => false, 'update_post_meta_cache' => false, 'meta_query' => $meta_query, );This keeps
found_postsaccurate but minimizes the result set and object hydration.
287-289: Fix typo in onboarding commentThe inline comment says “no oneboarding date”; should be “no onboarding date”. Cheap to fix and keeps the codebase tidy.
src/Settings/Settings.php (2)
553-560: Docblock default forget_onboarding_status()doesn’t match implementationThe docblock says the default is
ONBOARDING_PENDING_OPTION, but the method signature defaults toONBOARDING_COMPLETED_OPTIONand falls back toONBOARDING_PENDING_OPTIONonly when the stored value is invalid. Adjust the docblock to describe the actual behavior to avoid confusion when reading or generating IDE hints.
569-591:clear_all_options()leaves the onboarding date option behind
clear_all_options()now deletes onboarding-related keys (status, wizard step, completion) but notONBOARDING_DATE_KEY. Given the method’s purpose is to “Clear all the options”, this leftover value is surprising and could leak stale onboarding metadata after a reset. Consider also deletingself::ONBOARDING_DATE_KEYhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Dashboard/Dashboard_Notifications.php(1 hunks)src/Dashboard/Dashboard_Statistics.php(1 hunks)src/Dashboard/Setup_Wizard.php(9 hunks)src/Event/Scan_Posts_Event.php(1 hunks)src/Settings/Settings.php(5 hunks)templates/admin/dashboard/widget.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Dashboard/Dashboard_Notifications.php
🧰 Additional context used
🧬 Code graph analysis (4)
src/Dashboard/Setup_Wizard.php (3)
src/Event/Scan_Posts_Event.php (2)
Scan_Posts_Event(24-174)force_add_to_action_scheduler(72-90)src/Settings/Settings.php (12)
Settings(22-592)is_wizard_completed(486-488)get_onboarding_status(555-560)update_setup_wizard_step(475-477)set_onboarding_status(537-544)get_onboarding_date(510-513)set_onboarding_date(524-526)get_fixer_option(273-275)should_scan_existing_posts(211-217)set_wizard_completed(499-501)own_link_routinely_update(353-358)get_setup_wizard_step(462-464)src/Util/Environmental.php (2)
Environmental(20-38)is_production(27-37)
templates/admin/dashboard/widget.php (3)
src/Dashboard/Dashboard_Page.php (3)
Dashboard_Page(30-294)is_current_page(78-81)get_page_url(88-90)functions.php (1)
iawmlf_render_template(195-230)src/Dashboard/Report_Page.php (2)
Report_Page(23-361)get_page_url(58-60)
src/Dashboard/Dashboard_Statistics.php (3)
src/Link/Link.php (6)
Link(21-497)is_broken(180-182)get_id(118-120)has_archived_href(257-259)get_last_check(299-306)get_archive_process(372-374)src/Settings/Settings.php (3)
Settings(22-592)get_onboarding_date(510-513)get_allowed_post_types(101-103)src/Link/Link_Repository.php (2)
Link_Repository(23-673)query_links(394-510)
src/Event/Scan_Posts_Event.php (1)
src/Settings/Settings.php (2)
Settings(22-592)should_scan_existing_posts(211-217)
⏰ 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: build (ubuntu-latest, 8.3)
- GitHub Check: build (ubuntu-latest, 8.4)
- GitHub Check: build (ubuntu-latest, 8.1)
- GitHub Check: build (ubuntu-latest, 8.2)
🔇 Additional comments (1)
templates/admin/dashboard/widget.php (1)
153-159: Dashboard navigation button logic looks correctConditionally hiding the Dashboard button when already on the dashboard via
Dashboard_Page::is_current_page()avoids redundant links and keeps the nav consistent with the other buttons.
| private static function compile_link_statistics(): array { | ||
| $all_links = ( new Link_Repository() )->query_links( \PHP_INT_MAX, 1, array(), array(), array(), Link_Repository::ORDER_DATE_DESC, null, null, null ); | ||
|
|
||
| // Get all the links stats. | ||
| $all_broken = array(); | ||
| $redirected_broken = array(); | ||
| $has_archive_link = array(); | ||
| $not_checked = array(); | ||
| $process_done = array(); | ||
| $process_new = array(); | ||
| $process_pending = array(); | ||
| $last_checks = array(); | ||
|
|
||
| // Loop through all links to gather stats. | ||
| foreach ( $all_links as $link ) { | ||
| if ( $link->is_broken() && ! $link->is_excluded() ) { | ||
| $all_broken[] = $link->get_id(); | ||
| } | ||
|
|
||
| if ( $link->is_broken() && $link->has_archived_href() && ! $link->is_excluded() ) { | ||
| $redirected_broken[] = $link->get_id(); | ||
| } | ||
|
|
||
| if ( $link->has_archived_href() ) { | ||
| $has_archive_link[] = $link->get_id(); | ||
| } | ||
|
|
||
| if ( null === $link->get_last_check() ) { | ||
| $not_checked[] = $link->get_id(); | ||
| } else { | ||
| $last = $link->get_last_check(); | ||
| $last_checks[] = array( | ||
| 'id' => $link->get_id(), | ||
| 'last_check' => $last, | ||
| ); | ||
| } | ||
|
|
||
| switch ( $link->get_archive_process() ) { | ||
| case Link::PROCESS_NEW: | ||
| $process_new[] = $link->get_id(); | ||
| break; | ||
| case Link::PROCESS_PENDING: | ||
| $process_pending[] = $link->get_id(); | ||
| break; | ||
| default: | ||
| $process_done[] = $link->get_id(); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Sort the last checks by date desc. | ||
| usort( | ||
| $last_checks, | ||
| function ( $a, $b ) { | ||
| return strtotime( $b['last_check']['date'] ) <=> strtotime( $a['last_check']['date'] ); | ||
| } | ||
| ); | ||
|
|
||
| $stats = array( | ||
| 'total_links' => count( $all_links ), | ||
| 'all_broken_links' => count( $all_broken ), | ||
| 'broken_and_redirected_links' => count( $redirected_broken ), | ||
| 'broken_not_redirected_links' => count( $all_broken ) - count( $redirected_broken ), | ||
| 'links_with_archive' => count( $has_archive_link ), | ||
| 'links_without_archive' => count( $all_links ) - count( $has_archive_link ), | ||
| 'not_checked' => count( $not_checked ), | ||
| 'process_done' => count( $process_done ), | ||
| 'process_new' => count( $process_new ), | ||
| 'process_pending' => count( $process_pending ), | ||
| 'last_checks' => array_slice( $last_checks, 0, absint( apply_filters( 'iawmlf_dashboard_link_count', 10 ) ) ), | ||
| ); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Full-table link scan for stats may not scale on large sites
compile_link_statistics() pulls all links via query_links( \PHP_INT_MAX, 1, … ) and iterates them in PHP. On sites with a large iawmlf_link_archive table this will be heavy every time the transient is cold or invalid, and it runs inside admin requests. Consider aggregating via SQL (e.g., dedicated repository methods that return counts and last-check summaries only) or at least paging through the data instead of using PHP_INT_MAX.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine Link_Repository class and its query_links method
rg -n 'class Link_Repository' -A 5 --type=php
Length of output: 392
🏁 Script executed:
#!/bin/bash
# Find query_links method implementation
rg -n 'function query_links|public.*query_links' -A 15 --type=phpLength of output: 3289
🏁 Script executed:
#!/bin/bash
# Find get_link_statistics and compile_link_statistics to see caching logic
rg -n 'function get_link_statistics|function compile_link_statistics' -B 2 -A 20 --type=phpLength of output: 3558
🏁 Script executed:
#!/bin/bash
# Examine Link model methods referenced in the snippet
rg -n '(is_broken|is_excluded|has_archived_href|get_archive_process)' --type=php -B 2 -A 5 | head -100Length of output: 8469
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full query_links implementation
sed -n '394,500p' src/Link/Link_Repository.php
Length of output: 4173
🏁 Script executed:
#!/bin/bash
# Find Link model class and its method implementations
rg -n 'class Link' -A 3 --type=php | grep -A 3 'src/Link/Link.php'Length of output: 320
🏁 Script executed:
#!/bin/bash
# Check Link model methods: is_broken, is_excluded, has_archived_href
rg -n 'function is_broken|function is_excluded|function has_archived_href|function get_archive_process' -A 8 --type=php src/Link/Link.phpLength of output: 807
🏁 Script executed:
#!/bin/bash
# Find all calls to get_link_statistics to understand usage context
rg -n 'get_link_statistics' --type=php -B 2 -A 2Length of output: 1613
Performance concern is valid — full-table link scan in compile_link_statistics() will not scale
The method fetches all links with query_links( \PHP_INT_MAX, 1, … ), which executes a single SQL query with no LIMIT/OFFSET constraint and loads every link as a Link object into memory. On sites with large iawmlf_link_archive tables, this causes:
- Single massive database query (all rows fetched at once)
- High memory usage from object hydration
- Full PHP loop iteration on every cache miss
Since the transient expires every 2 minutes and this runs on dashboard page load, cache-cold scenarios will trigger expensive recalculation repeatedly.
Recommendation: Implement SQL-side aggregation (e.g., repository methods returning COUNT, GROUP BY results) instead of fetching and looping all rows in PHP. Alternatively, page through data in chunks and aggregate within the loop to keep memory bounded.
1 participant
Changes proposed in this Pull Request
This pull request introduces a refactor and enhancement of the onboarding and setup wizard logic for the plugin, centralizing state management in the
Settingsclass and improving user experience for onboarding and wizard steps. It also adds UI improvements and code cleanups related to the wizard flow.Onboarding & Wizard Logic Refactor:
Settingsclass, replacing previous direct option usage and scattered logic. This includes new methods likeget_setup_wizard_step,update_setup_wizard_step,is_wizard_completed,set_wizard_completed,set_onboarding_status, andget_onboarding_status. [1] [2] [3]Settingsmethods, replacing previous logic and constants inSetup_Wizard. This includes changes to how steps are marked as complete, how onboarding is triggered, and how the wizard is rerun or redirected. [1] [2] [3] [4] [5] [6] [7] [8] [9]Onboarding Experience Improvements:
iawmlf-onboarding-active) for styling and a redirect to the wizard if onboarding is pending. [1] [2] [3]Wizard UI & Usability:
disabledattribute. [1] [2]Code Cleanup & Consistency:
Setup_Wizardhave been removed, and step handling logic has been simplified to use the new settings methods. [1] [2]Settings::clear_all_options()now removes onboarding-related options.Styling Enhancements:
Testing instructions
Mentions #265
Summary by CodeRabbit
New Features
Improvements
Chores
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.