first draft for misc settings#672
Merged
Akhill2020 merged 15 commits intomainfrom May 7, 2026
Merged
Conversation
* Design system First draft (#648) * Design system First draft * Feedback changes * feddback changes in css * feedback changes * required images * make scss & local fonts * input text css * removed the dev api and enter random key * Connect page settings * feedback changes except js * feedback changes for js and other
* Design system First draft (#648) * Design system First draft * Feedback changes * feddback changes in css * feedback changes * required images * make scss & local fonts * input text css * removed the dev api and enter random key * Connect page settings * feedback changes except js * feedback changes for js and other * pro related update * imimprovement in the pro
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a new "Misc Settings" admin page with dedicated styling and layout, extracts Connect sidebar/onboarding state preparation into reusable utility functions, centralizes Google Calendar Pro detection logic, and updates asset registration and multiple Connect components to use the new functions. ChangesAdmin Interface & Connect Sidebar Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
* Design system First draft (#648) * Design system First draft * Feedback changes * feddback changes in css * feedback changes * required images * make scss & local fonts * input text css * removed the dev api and enter random key * Connect page settings * feedback changes except js * feedback changes for js and other * pro related update * imimprovement in the pro * Pro addon steps feedback, Notice & version * QA feed back * Undo es build changes * Feedback changes * QA feedback fixes * remove unwanted action link * add new calendar button conidtion update * connect menu display issue * lint issue fix
…e-Calendar into revamp-setting_page
Akhill2020
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
includes/admin/pages.php (1)
500-502:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOrphan
</form>tag will be output on the misc settings page.The
</form>on line 501 is outside theelseblock that opens the form (line 324). When$is_misc_settings_pageis true, the code takes the first branch (lines 228-321) which has its own forms inside each card, but line 501's</form>will still be output, creating invalid HTML.🐛 Proposed fix - move inside the else block
<?php } echo '</div>'; echo '</div>'; - }?> - </form> + ?> + </form> + <?php }?> </div>This ensures
</form>only outputs when the form was actually opened in the else branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/admin/pages.php` around lines 500 - 502, The orphan closing </form> is rendered even when $is_misc_settings_page is true because the opening <form> is inside the else branch; move the orphaned </form> into the same else block that opens the form so it is only emitted when the form was created (ensure the closing tag sits after the controls generated in the else branch, pairing with that form); reference $is_misc_settings_page and the existing form block to locate where to relocate the </form>.includes/admin/pages/connect/partials/google-calendar-api-key-connect.php (1)
164-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden and escape the Pro download link output.
Line 168 uses
target="_blank"withoutrel="noopener noreferrer", and Line 164-168 should escape the URL output.Suggested patch
- <a href="<?php echo simcal_ga_campaign_url( + <a href="<?php echo esc_url(simcal_ga_campaign_url( 'https://simplecalendar.io/downloads/google-calendar-pro/', 'core-plugin', 'connect-api-key-pro-addon', - ); ?>" target="_blank" class="sc_connect_pro_link"> + )); ?>" target="_blank" rel="noopener noreferrer" class="sc_connect_pro_link">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/admin/pages/connect/partials/google-calendar-api-key-connect.php` around lines 164 - 168, Wrap the simcal_ga_campaign_url(...) call used inside the anchor href with an appropriate escaping function (e.g., esc_url()) and ensure the href attribute uses the escaped value; also add rel="noopener noreferrer" to the anchor tag (the <a> with class sc_connect_pro_link) while keeping target="_blank". Update the anchor that calls simcal_ga_campaign_url to use the escaped URL and include rel attributes to harden the external link.includes/admin/pages/connect/partials/helpful-links.php (1)
34-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete new-tab hardening for the other external links.
Line 34 and Line 50 still use
target="_blank"withoutrel="noopener noreferrer", and Line 34 should URL-escape output as well.Suggested patch
- <a href="<?php echo simcal_ga_campaign_url( + <a href="<?php echo esc_url(simcal_ga_campaign_url( 'https://docs.simplecalendar.io/', 'core-plugin', 'connect-documentation', - ); ?>" target="_blank" class="sc_helpful_link_card"> + )); ?>" target="_blank" rel="noopener noreferrer" class="sc_helpful_link_card"> @@ - <a href="<?php echo esc_url($support_url); ?>" target="_blank" class="sc_helpful_link_card"> + <a href="<?php echo esc_url($support_url); ?>" target="_blank" rel="noopener noreferrer" class="sc_helpful_link_card">Also applies to: 50-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/admin/pages/connect/partials/helpful-links.php` around lines 34 - 38, The anchor using simcal_ga_campaign_url currently outputs an unescaped URL and uses target="_blank" without link hardening; update the anchor(s) that call simcal_ga_campaign_url (and any other external anchors in this file) to wrap the URL output with esc_url() (or equivalent URL-escaping) and add rel="noopener noreferrer" to the tag, e.g., replace echo simcal_ga_campaign_url(...) with echo esc_url( simcal_ga_campaign_url(...) ) and ensure each <a ... target="_blank"> also includes rel="noopener noreferrer".includes/admin/pages/connect/steps/pro-credentials.php (1)
192-208:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden helper links opened in new tabs and escape URLs.
Line 192 and Line 204 output external links with
target="_blank"but norel="noopener noreferrer", and the injected URL values should be escaped withesc_url(...).Suggested patch
echo sprintf( __( - '<a href="%s" target="_blank" class="sc_connect_helper_link">Step-by-step instructions</a>', + '<a href="%s" target="_blank" rel="noopener noreferrer" class="sc_connect_helper_link">Step-by-step instructions</a>', 'google-calendar-events', ), - simcal_ga_campaign_url( + esc_url(simcal_ga_campaign_url( simcal_get_url('docs') . '/google-calendar-pro-configure-google-oauth/', 'gcal-pro', 'settings-link', - ), + )), ); echo sprintf( __( - '<a href="%s" target="_blank" class="sc_connect_helper_link">Google Developers Console</a> ', + '<a href="%s" target="_blank" rel="noopener noreferrer" class="sc_connect_helper_link">Google Developers Console</a> ', 'google-calendar-events', ), - simcal_get_url('gdev-console'), + esc_url(simcal_get_url('gdev-console')), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/admin/pages/connect/steps/pro-credentials.php` around lines 192 - 208, Escape the external URLs and add rel="noopener noreferrer" to the anchor outputs: pass escaped URLs into simcal_ga_campaign_url by calling esc_url( simcal_get_url('docs') . '/google-calendar-pro-configure-google-oauth/' ) when invoking simcal_ga_campaign_url (so the URL arg is safe), and update the echo sprintf that prints the Google Developers Console link to use esc_url( simcal_get_url('gdev-console') ) and include rel="noopener noreferrer" inside the '<a ...>' template string (keeping target="_blank" and class="sc_connect_helper_link"); adjust the call sites to use these escaped values and the added rel attribute.
🧹 Nitpick comments (1)
includes/admin/pages.php (1)
236-259: 💤 Low valueVariables
$progressand$force_complete_stylesappear unused in this file.These variables are defined but not used directly in this template. If they're expected to be consumed by the included
progress.phptemplate (line 318), consider adding a brief comment to clarify this dependency for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/admin/pages.php` around lines 236 - 259, The $progress array and $force_complete_styles boolean are defined here to be consumed by the later included template (progress.php) but appear unused in this file; add a concise inline comment above the definitions (referencing $progress, $force_complete_styles and progress.php) explaining that these variables are intentionally set here to provide data to the included progress.php template so future maintainers don’t remove or refactor them—keep the variable names as-is and ensure the include at progress.php still relies on current scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Around line 81-83: The composer script "build-admin-css" currently runs "npm
run build:admin-ui-css" but that npm script doesn't exist; update the composer
script to invoke the existing "build:esbuild" npm script (i.e., change the
command to "npm run build:esbuild") so SCSS entry points like misc-settings.scss
get compiled, or alternatively add a new npm script named "build:admin-ui-css"
to package.json that delegates to the appropriate build step; locate the
"build-admin-css" key in composer.json and either replace the referenced npm
script or add the missing npm script in package.json.
In `@includes/admin/assets.php`:
- Around line 59-64: The computed $admin_js_ver is never used and the
is_string($admin_js_file) check is redundant; update the wp_register_script call
that currently passes SIMPLE_CALENDAR_VERSION to use $admin_js_ver instead so
the file mtime is used for cache-busting, and remove the unnecessary is_string()
check around file_exists() since $admin_js_file is a string literal (or delete
the whole $admin_js_ver block if you prefer not to use mtime-based versioning);
target the variables $admin_js_ver, $admin_js_file and the wp_register_script
invocation to make this change.
In `@includes/admin/pages.php`:
- Line 272: Escape the description output to prevent XSS by wrapping the
unescaped $calendars_contents['description'] (and the other identical occurrence
near the bottom using the same variable) with a proper escaping function—use
wp_kses_post() if HTML is allowed or esc_html() for plain text—so replace the
direct echo of $calendars_contents['description'] in the template with the
chosen escaped output (update both instances referenced in the diff).
In `@includes/admin/pages/connect/partials/helpful-links.php`:
- Line 44: Prettier/formatting failed for
includes/admin/pages/connect/partials/helpful-links.php; run your project's
configured formatter (Prettier or php-cs-fixer as used by CI) on that file, fix
any formatting issues, and commit the reformatted file so the line with the
anchor referencing $setup_video_url and the class sc_helpful_link_card matches
the repository’s style rules.
In `@includes/admin/pages/connect/steps/pro-credentials.php`:
- Around line 242-253: The failing Prettier check is caused by inconsistent
formatting of the two esc_html_e(...) calls in the pro-credentials.php snippet;
run the project's formatter (Prettier/PHPCS configured for the repo) and
reformat the two calls to match project style so the argument strings and domain
are aligned consistently (look for esc_html_e in the two <p
class="sc_text--body_b3 sc_text--dark sc_connect_credentials_step_help"> blocks)
— ensure indentation, line breaks, and trailing commas follow the repo's
formatting rules and then commit the formatted file.
---
Outside diff comments:
In `@includes/admin/pages.php`:
- Around line 500-502: The orphan closing </form> is rendered even when
$is_misc_settings_page is true because the opening <form> is inside the else
branch; move the orphaned </form> into the same else block that opens the form
so it is only emitted when the form was created (ensure the closing tag sits
after the controls generated in the else branch, pairing with that form);
reference $is_misc_settings_page and the existing form block to locate where to
relocate the </form>.
In `@includes/admin/pages/connect/partials/google-calendar-api-key-connect.php`:
- Around line 164-168: Wrap the simcal_ga_campaign_url(...) call used inside the
anchor href with an appropriate escaping function (e.g., esc_url()) and ensure
the href attribute uses the escaped value; also add rel="noopener noreferrer" to
the anchor tag (the <a> with class sc_connect_pro_link) while keeping
target="_blank". Update the anchor that calls simcal_ga_campaign_url to use the
escaped URL and include rel attributes to harden the external link.
In `@includes/admin/pages/connect/partials/helpful-links.php`:
- Around line 34-38: The anchor using simcal_ga_campaign_url currently outputs
an unescaped URL and uses target="_blank" without link hardening; update the
anchor(s) that call simcal_ga_campaign_url (and any other external anchors in
this file) to wrap the URL output with esc_url() (or equivalent URL-escaping)
and add rel="noopener noreferrer" to the tag, e.g., replace echo
simcal_ga_campaign_url(...) with echo esc_url( simcal_ga_campaign_url(...) ) and
ensure each <a ... target="_blank"> also includes rel="noopener noreferrer".
In `@includes/admin/pages/connect/steps/pro-credentials.php`:
- Around line 192-208: Escape the external URLs and add rel="noopener
noreferrer" to the anchor outputs: pass escaped URLs into simcal_ga_campaign_url
by calling esc_url( simcal_get_url('docs') .
'/google-calendar-pro-configure-google-oauth/' ) when invoking
simcal_ga_campaign_url (so the URL arg is safe), and update the echo sprintf
that prints the Google Developers Console link to use esc_url(
simcal_get_url('gdev-console') ) and include rel="noopener noreferrer" inside
the '<a ...>' template string (keeping target="_blank" and
class="sc_connect_helper_link"); adjust the call sites to use these escaped
values and the added rel attribute.
---
Nitpick comments:
In `@includes/admin/pages.php`:
- Around line 236-259: The $progress array and $force_complete_styles boolean
are defined here to be consumed by the later included template (progress.php)
but appear unused in this file; add a concise inline comment above the
definitions (referencing $progress, $force_complete_styles and progress.php)
explaining that these variables are intentionally set here to provide data to
the included progress.php template so future maintainers don’t remove or
refactor them—keep the variable names as-is and ensure the include at
progress.php still relies on current scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54f611e2-0b5c-4382-9be5-650bd8ad7c07
📒 Files selected for processing (11)
.bin/esbuild.mjsassets/js/admin.jsassets/scss/misc-settings.scsscomposer.jsonincludes/admin/assets.phpincludes/admin/metaboxes/settings.phpincludes/admin/pages.phpincludes/admin/pages/connect/layout.phpincludes/admin/pages/connect/partials/google-calendar-api-key-connect.phpincludes/admin/pages/connect/partials/helpful-links.phpincludes/admin/pages/connect/steps/pro-credentials.php
💤 Files with no reviewable changes (1)
- assets/js/admin.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
includes/admin/pages.php (1)
286-288:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape
$contents['description']before echoing.
$calendars_contents['description'](line 287) and$advanced_contents['description'](line 312) are echoed without escaping. Usewp_kses_post()if HTML is intentionally allowed in these descriptions, otherwiseesc_html().🛡️ Proposed fix
- <?php if (!empty($calendars_contents['description'])) { ?> - <div class="sc_text--body_b3 sc_text--dark"><?php echo $calendars_contents['description']; ?></div> - <?php } ?> + <?php if (!empty($calendars_contents['description'])) { ?> + <div class="sc_text--body_b3 sc_text--dark"><?php echo wp_kses_post($calendars_contents['description']); ?></div> + <?php } ?>- <?php if (!empty($advanced_contents['description'])) { ?> - <div class="sc_text--body_b3 sc_text--dark"><?php echo $advanced_contents['description']; ?></div> - <?php } ?> + <?php if (!empty($advanced_contents['description'])) { ?> + <div class="sc_text--body_b3 sc_text--dark"><?php echo wp_kses_post($advanced_contents['description']); ?></div> + <?php } ?>Also applies to: 311-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/admin/pages.php` around lines 286 - 288, Escape the description output before echoing: replace direct echoes of $calendars_contents['description'] and $advanced_contents['description'] with a sanitized version using either wp_kses_post($calendars_contents['description']) / wp_kses_post($advanced_contents['description']) if HTML is allowed, or esc_html($calendars_contents['description']) / esc_html($advanced_contents['description']) if only plain text is allowed; update the echo sites in the template where these variables are printed (the blocks rendering the description) to use the chosen escaping function.
🧹 Nitpick comments (3)
includes/functions/admin.php (3)
453-507: ⚡ Quick winConsider memoizing the scope to avoid repeated
WP_Querycalls per request.This helper runs up to three
WP_Querycalls (taxonomycalendar_feed,_feed_typemeta_query, genericcalendarcount) plus severalget_optionreads, and is invoked from bothconnect-controller.phpand the new misc-settings render path. A static request-level cache (orwp_cache_*for short-lived caching) would reduce DB pressure on admins who navigate between Connect and Misc Settings, and also future-proof additional callers.♻️ Sketch: request-scoped memoization
function simcal_prepare_connect_sidebar_scope() { + static $cached_scope = null; + if (null !== $cached_scope) { + return $cached_scope; + } $welcome_context = (string) get_option('simple_calendar_connect_welcome_context', ''); ... - return [ ... ]; + $cached_scope = [ ... ]; + return $cached_scope; }Note: if you keep the side-effects (OAuth token persistence, completion-timestamp writes) inline, those should ideally run only on the first call — the static-cache pattern naturally enforces that.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/functions/admin.php` around lines 453 - 507, This code runs up to three WP_Query calls and repeated option reads each request; wrap the logic that builds $pro_query_base, the taxonomy/meta queries (the WP_Query instances creating $pro_calendar_query) and the final calendar query into a request-scoped memo so subsequent calls return cached results: implement a static variable (e.g. $cached_calendar_checks) at the top of the containing function in includes/functions/admin.php (or use wp_cache_get/wp_cache_set with a short TTL) to store the computed flags ($has_published_pro_calendar, $has_published_calendar) and any option-derived values, return cached values immediately on subsequent invocations, and ensure any side-effects (OAuth token persistence or completion-timestamp writes) are executed only once when populating the cache; also only call wp_reset_postdata() for queries you actually ran.
372-405: 💤 Low valueDead reinitialization of
$is_pro_active.
$is_pro_activeis set tofalseat line 372, then set tofalseagain at line 381 with no read in between — the first assignment is dead. The interleavedis_plugin_activebootstrap can stand on its own.♻️ Proposed cleanup
- $is_pro_active = false; if (!function_exists('is_plugin_active') && defined('ABSPATH')) { $plugin_file = trailingslashit(ABSPATH) . 'wp-admin/includes/plugin.php'; if (is_readable($plugin_file)) { // phpcs:ignore WordPressVIPMinimum.Files.IncludingFile.UsingVariable require_once $plugin_file; } } $is_pro_active = false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/functions/admin.php` around lines 372 - 405, The initial assignment $is_pro_active = false; at the top is dead and should be removed; keep a single initialization before the feature-detection block (the later $is_pro_active = false;), preserve the is_plugin_active bootstrap (the require_once of wp-admin/includes/plugin.php) and the subsequent detection logic (class_exists('Google_Pro'), defined('SIMPLE_CALENDAR_GOOGLE_PRO_VERSION'), the is_plugin_active checks, the fallback using get_option('active_plugins') and array_map, and the final cast/apply_filters call), so delete the redundant first assignment and leave the rest unchanged to avoid duplicate initialization.
412-436: ⚖️ Poor tradeoffMove OAuth token processing to a dedicated
admin_inithandler scoped to the Connect page.The
auth_tokenside-effect executes wheneversimcal_prepare_connect_sidebar_scope()is called. Because this function is now invoked on the Misc Settings page (includes/admin/pages.php:250), any request to/edit.php?post_type=calendar&page=simple-calendar_misc_settings&auth_token=…will triggerupdate_option('simple_calendar_auth_site_token', …).While the nonce +
manage_options+ referer checks prevent exploitation, coupling OAuth token handling inside a sidebar scope helper conflates concerns and unnecessarily widens the surface area where this side-effect fires. Extract this block into a separateadmin_inithandler scoped only to the Connect page, keeping the sidebar helper read-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/functions/admin.php` around lines 412 - 436, The OAuth token handling currently inside simcal_prepare_connect_sidebar_scope should be moved out into a dedicated admin_init handler that only runs on the Connect page: create a new function (e.g. simcal_handle_connect_oauth_token) hooked to admin_init that first checks is_admin(), verifies $_GET['page'] === 'simple-calendar_connect' (or uses get_current_screen()->id) then performs the same sanitized auth_token/state extraction, wp_verify_nonce('simcal_oauth_via_sc_state'), referer origin check, current_user_can('manage_options') and finally update_option('simple_calendar_auth_site_token', …) and update_option('simple_calendar_connect_pro_connection_type','via_sc') when $is_pro_active; remove the entire auth_token side-effect block from simcal_prepare_connect_sidebar_scope so that the sidebar helper becomes read-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@includes/admin/pages.php`:
- Around line 286-288: Escape the description output before echoing: replace
direct echoes of $calendars_contents['description'] and
$advanced_contents['description'] with a sanitized version using either
wp_kses_post($calendars_contents['description']) /
wp_kses_post($advanced_contents['description']) if HTML is allowed, or
esc_html($calendars_contents['description']) /
esc_html($advanced_contents['description']) if only plain text is allowed;
update the echo sites in the template where these variables are printed (the
blocks rendering the description) to use the chosen escaping function.
---
Nitpick comments:
In `@includes/functions/admin.php`:
- Around line 453-507: This code runs up to three WP_Query calls and repeated
option reads each request; wrap the logic that builds $pro_query_base, the
taxonomy/meta queries (the WP_Query instances creating $pro_calendar_query) and
the final calendar query into a request-scoped memo so subsequent calls return
cached results: implement a static variable (e.g. $cached_calendar_checks) at
the top of the containing function in includes/functions/admin.php (or use
wp_cache_get/wp_cache_set with a short TTL) to store the computed flags
($has_published_pro_calendar, $has_published_calendar) and any option-derived
values, return cached values immediately on subsequent invocations, and ensure
any side-effects (OAuth token persistence or completion-timestamp writes) are
executed only once when populating the cache; also only call wp_reset_postdata()
for queries you actually ran.
- Around line 372-405: The initial assignment $is_pro_active = false; at the top
is dead and should be removed; keep a single initialization before the
feature-detection block (the later $is_pro_active = false;), preserve the
is_plugin_active bootstrap (the require_once of wp-admin/includes/plugin.php)
and the subsequent detection logic (class_exists('Google_Pro'),
defined('SIMPLE_CALENDAR_GOOGLE_PRO_VERSION'), the is_plugin_active checks, the
fallback using get_option('active_plugins') and array_map, and the final
cast/apply_filters call), so delete the redundant first assignment and leave the
rest unchanged to avoid duplicate initialization.
- Around line 412-436: The OAuth token handling currently inside
simcal_prepare_connect_sidebar_scope should be moved out into a dedicated
admin_init handler that only runs on the Connect page: create a new function
(e.g. simcal_handle_connect_oauth_token) hooked to admin_init that first checks
is_admin(), verifies $_GET['page'] === 'simple-calendar_connect' (or uses
get_current_screen()->id) then performs the same sanitized auth_token/state
extraction, wp_verify_nonce('simcal_oauth_via_sc_state'), referer origin check,
current_user_can('manage_options') and finally
update_option('simple_calendar_auth_site_token', …) and
update_option('simple_calendar_connect_pro_connection_type','via_sc') when
$is_pro_active; remove the entire auth_token side-effect block from
simcal_prepare_connect_sidebar_scope so that the sidebar helper becomes
read-only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68339b6a-3f44-492c-a242-08f9b3d83edd
📒 Files selected for processing (8)
assets/scss/misc-settings.scssincludes/admin/assets.phpincludes/admin/pages.phpincludes/admin/pages/connect-controller.phpincludes/admin/pages/connect/partials/helpful-links.phpincludes/admin/pages/connect/steps/pro-credentials.phpincludes/functions/admin.phpincludes/main.php
✅ Files skipped from review due to trivial changes (1)
- assets/scss/misc-settings.scss
rosinghal
reviewed
May 5, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
includes/admin/pages/connect-controller.php (1)
17-20: ⚡ Quick win
$assets_baseon line 17 is now dead, andextract()hides variable origin.
extract($scope)at line 20 overwrites the$assets_baseassigned on line 17 (the scope already provides'assets_base' => SIMPLE_CALENDAR_ASSETS . 'images/admin/'), so line 17 has no effect. Additionally, callingextract()with no flag defaults toEXTR_OVERWRITEand silently introduces a dozen locals ($api_key,$is_pro_flow,$has_published_calendar, …) consumed below at lines 26, 30, 56–63, 65, 78, 80; readers can no longer trace where each variable came from, and any future local of the same name gets clobbered.♻️ Proposed cleanup
-$assets_base = SIMPLE_CALENDAR_ASSETS . 'images/admin/'; - $scope = simcal_prepare_connect_sidebar_scope(); -extract($scope); +$welcome_context = $scope['welcome_context']; +$is_pro_flow = $scope['is_pro_flow']; +$api_key = $scope['api_key']; +$has_api_key = $scope['has_api_key']; +$has_core_api_key_verified = $scope['has_core_api_key_verified']; +$has_oauth_connection = $scope['has_oauth_connection']; +$has_client_credentials = $scope['has_client_credentials']; +$has_published_pro_calendar = $scope['has_published_pro_calendar']; +$has_published_calendar = $scope['has_published_calendar']; +$should_hide_progress = $scope['should_hide_progress']; +$assets_base = $scope['assets_base'];If you want to keep the conciseness of
extract(), at minimum passEXTR_SKIPso future locals defined above this line aren't accidentally overwritten.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/admin/pages/connect-controller.php` around lines 17 - 20, The assignment to $assets_base is being overwritten by extract($scope) returned from simcal_prepare_connect_sidebar_scope(), and extract() with default EXTR_OVERWRITE hides variable origins and can clobber locals; change the call to use extract($scope, EXTR_SKIP) (or avoid extract entirely and pull needed keys from the array) so the $assets_base set before the call is preserved and to prevent silent overwrites of locals like $api_key, $is_pro_flow, $has_published_calendar used later in this file.includes/admin/pages/connect/steps/pro-credentials.php (2)
14-17: 💤 Low valueDrop the dead initial assignment (same pattern as
core-credentials.php).-$is_pro_active = false; $is_pro_active = function_exists('simcal_is_google_calendar_pro_active') ? simcal_is_google_calendar_pro_active(isset($welcome_context) ? (string) $welcome_context : '') : isset($welcome_context) && 'pro' === (string) $welcome_context;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/admin/pages/connect/steps/pro-credentials.php` around lines 14 - 17, Remove the redundant initial assignment to $is_pro_active; delete the line "$is_pro_active = false;" so that $is_pro_active is only set once by the conditional expression that calls simcal_is_google_calendar_pro_active (using isset($welcome_context) ? (string) $welcome_context : '') or falls back to the welcome_context check ('pro' === (string) $welcome_context). This makes the variable initialization match the pattern used in core-credentials.php and avoids the dead assignment.
128-130: 💤 Low valueDuplicate comment — collapse the repeated sentence.
The "Always render the status pill (unlink + Not Connected) like the design." line appears twice (line 128 inside
<?php //and again on line 129).♻️ Proposed cleanup
- <?php // Always render the status pill (unlink + Not Connected) like the design. - // Always render the status pill (unlink + Not Connected) like the design. - // Only enable AJAX checking when a token exists, to avoid showing network errors when not authenticated. + <?php + // Always render the status pill (unlink + Not Connected) like the design. + // Only enable AJAX checking when a token exists, to avoid showing network errors when not authenticated. $auth_status_text = !empty($has_oauth_connection)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/admin/pages/connect/steps/pro-credentials.php` around lines 128 - 130, Remove the duplicated inline comment "Always render the status pill (unlink + Not Connected) like the design." in includes/admin/pages/connect/steps/pro-credentials.php so it appears only once; locate the twin PHP single-line comments near the top of the block (the two "<?php // Always render the status pill..." lines) and delete the redundant one, preserving the remaining comment and the subsequent comment about AJAX checking.includes/functions/admin.php (1)
442-465: 🏗️ Heavy liftOAuth callback handling buried inside a "prepare scope" helper.
Persisting
simple_calendar_auth_site_token(andsimple_calendar_connect_pro_connection_type) is a state-changing side effect that now triggers on any admin page that calls this helper, including the misc-settings page. The capability + nonce + helper-origin checks make it safe, but the coupling is surprising: a function that returns a view scope shouldn't also be the OAuth landing handler.Consider extracting this block into a dedicated function (e.g.
simcal_handle_oauth_callback()) hooked onadmin_init(or invoked only from the Connect page), and letsimcal_prepare_connect_sidebar_scope()only read the resulting options. That also avoids the redundant re-processing pointed out in the memoization comment above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/functions/admin.php` around lines 442 - 465, Extract the OAuth callback handling out of simcal_prepare_connect_sidebar_scope(): move the entire block that reads $_GET['auth_token'] / $_GET['state'], performs helper-origin, nonce and current_user_can checks, and updates the options (simple_calendar_auth_site_token and simple_calendar_connect_pro_connection_type) into a new function named simcal_handle_oauth_callback(), preserving the same validations (SIMPLE_CALENDAR_OAUTH_HELPER_AUTH_DOMAIN, wp_parse_url, wp_get_raw_referer, wp_verify_nonce, current_user_can). Hook simcal_handle_oauth_callback() to admin_init (or call it only from the Connect page) so simcal_prepare_connect_sidebar_scope() only reads the options and does not perform side effects or re-process the callback.includes/admin/pages/connect/steps/core-credentials.php (1)
14-17: 💤 Low valueDrop the dead initial assignment.
Line 14 sets
$is_pro_active = false;and line 15 immediately overwrites it via the ternary — the first assignment is unreachable and just adds noise.♻️ Proposed cleanup
-$is_pro_active = false; $is_pro_active = function_exists('simcal_is_google_calendar_pro_active') ? simcal_is_google_calendar_pro_active(isset($welcome_context) ? (string) $welcome_context : '') : isset($welcome_context) && 'pro' === (string) $welcome_context;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/admin/pages/connect/steps/core-credentials.php` around lines 14 - 17, Remove the redundant initial assignment to $is_pro_active; instead rely only on the ternary expression that follows which computes $is_pro_active using function_exists('simcal_is_google_calendar_pro_active') ? simcal_is_google_calendar_pro_active(...) : ... and $welcome_context — delete the unreachable "$is_pro_active = false;" line so $is_pro_active is defined only by the ternary logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@includes/functions/admin.php`:
- Around line 427-588: Add request-level memoization inside
simcal_prepare_connect_sidebar_scope(): declare a static $cached (or similar) at
the top, return it immediately if non-null, and before the final return assign
the computed array to that static so subsequent calls in the same request reuse
the result; ensure you place the static check at the very start of the function
and assign the computed $result (or $scope) to the static before returning so
the existing OAuth update_option and setup-complete update logic still run only
once per request.
- Around line 374-399: The multisite network-activated Pro add-on isn't being
detected because the fallback only checks active_plugins; update the detection
in the block that sets $is_pro_active (and the is_plugin_active fallback) to
also check get_option('active_sitewide_plugins', []) when is_plugin_active() is
not available or if the manual loop fails: retrieve active_sitewide_plugins
(cast to array/strings), and scan those keys/filenames for 'google-calendar-pro'
or 'simple-calendar-google-calendar-pro' (mirroring the existing substring
checks) and set $is_pro_active = true if found; ensure you reference the same
symbols ($is_pro_active, is_plugin_active,
get_option('active_sitewide_plugins')) so multisite network-activated installs
are detected.
---
Nitpick comments:
In `@includes/admin/pages/connect-controller.php`:
- Around line 17-20: The assignment to $assets_base is being overwritten by
extract($scope) returned from simcal_prepare_connect_sidebar_scope(), and
extract() with default EXTR_OVERWRITE hides variable origins and can clobber
locals; change the call to use extract($scope, EXTR_SKIP) (or avoid extract
entirely and pull needed keys from the array) so the $assets_base set before the
call is preserved and to prevent silent overwrites of locals like $api_key,
$is_pro_flow, $has_published_calendar used later in this file.
In `@includes/admin/pages/connect/steps/core-credentials.php`:
- Around line 14-17: Remove the redundant initial assignment to $is_pro_active;
instead rely only on the ternary expression that follows which computes
$is_pro_active using function_exists('simcal_is_google_calendar_pro_active') ?
simcal_is_google_calendar_pro_active(...) : ... and $welcome_context — delete
the unreachable "$is_pro_active = false;" line so $is_pro_active is defined only
by the ternary logic.
In `@includes/admin/pages/connect/steps/pro-credentials.php`:
- Around line 14-17: Remove the redundant initial assignment to $is_pro_active;
delete the line "$is_pro_active = false;" so that $is_pro_active is only set
once by the conditional expression that calls
simcal_is_google_calendar_pro_active (using isset($welcome_context) ? (string)
$welcome_context : '') or falls back to the welcome_context check ('pro' ===
(string) $welcome_context). This makes the variable initialization match the
pattern used in core-credentials.php and avoids the dead assignment.
- Around line 128-130: Remove the duplicated inline comment "Always render the
status pill (unlink + Not Connected) like the design." in
includes/admin/pages/connect/steps/pro-credentials.php so it appears only once;
locate the twin PHP single-line comments near the top of the block (the two
"<?php // Always render the status pill..." lines) and delete the redundant one,
preserving the remaining comment and the subsequent comment about AJAX checking.
In `@includes/functions/admin.php`:
- Around line 442-465: Extract the OAuth callback handling out of
simcal_prepare_connect_sidebar_scope(): move the entire block that reads
$_GET['auth_token'] / $_GET['state'], performs helper-origin, nonce and
current_user_can checks, and updates the options
(simple_calendar_auth_site_token and
simple_calendar_connect_pro_connection_type) into a new function named
simcal_handle_oauth_callback(), preserving the same validations
(SIMPLE_CALENDAR_OAUTH_HELPER_AUTH_DOMAIN, wp_parse_url, wp_get_raw_referer,
wp_verify_nonce, current_user_can). Hook simcal_handle_oauth_callback() to
admin_init (or call it only from the Connect page) so
simcal_prepare_connect_sidebar_scope() only reads the options and does not
perform side effects or re-process the callback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4b2468e-c1aa-47c9-85ca-1ea91131df4d
📒 Files selected for processing (5)
includes/admin/pages/connect-controller.phpincludes/admin/pages/connect/partials/helpful-links.phpincludes/admin/pages/connect/steps/core-credentials.phpincludes/admin/pages/connect/steps/pro-credentials.phpincludes/functions/admin.php
rosinghal
reviewed
May 5, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
includes/functions/admin.php (1)
14-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
usestatements at lines 15–16.The imports
SimpleCalendar\Add_On_Google_ProandSimpleCalendar\Simple_Calendar_Appointmentare never referenced in this file. The Pro detection at lines 364–366 checks forGoogle_Proand\SimpleCalendar\Feeds\Google_Proinstead—different class names that are not imported here. Remove these unused imports to keep the code clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/functions/admin.php` around lines 14 - 16, Remove the unused import statements for SimpleCalendar\Add_On_Google_Pro and SimpleCalendar\Simple_Calendar_Appointment in includes/functions/admin.php; keep SimpleCalendar\Admin\Notice if used, and ensure Pro detection logic that checks for Google_Pro and \SimpleCalendar\Feeds\Google_Pro remains unchanged—this cleans up unused use() declarations for Add_On_Google_Pro and Simple_Calendar_Appointment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@includes/functions/admin.php`:
- Around line 14-16: Remove the unused import statements for
SimpleCalendar\Add_On_Google_Pro and SimpleCalendar\Simple_Calendar_Appointment
in includes/functions/admin.php; keep SimpleCalendar\Admin\Notice if used, and
ensure Pro detection logic that checks for Google_Pro and
\SimpleCalendar\Feeds\Google_Pro remains unchanged—this cleans up unused use()
declarations for Add_On_Google_Pro and Simple_Calendar_Appointment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a51b11b3-e292-4b5b-bade-e2115be3ec14
📒 Files selected for processing (4)
includes/admin/pages/connect/partials/helpful-links.phpincludes/admin/pages/connect/steps/core-credentials.phpincludes/admin/pages/connect/steps/pro-credentials.phpincludes/functions/admin.php
🚧 Files skipped from review as they are similar to previous changes (3)
- includes/admin/pages/connect/steps/core-credentials.php
- includes/admin/pages/connect/partials/helpful-links.php
- includes/admin/pages/connect/steps/pro-credentials.php
rosinghal
approved these changes
May 5, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
assets/scss/misc-settings.scss (1)
65-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
td { display: flex }breaks the conventional description layout.WordPress'
.form-tableconvention places a<p class="description">inside the same<td>as the input, expecting it to render on a new line below. Withdisplay: flex(defaultflex-direction: row), the description will sit horizontally next to the input, and themargin-top: 6pxyou've set on.descriptionwill not behave as intended.If you want the icon/help adornment to sit beside the input but still keep description below, switch the column direction or scope flex to a wrapper:
♻️ Suggested adjustment
td { padding: 0; vertical-align: top; - display: flex; - align-items: center; - gap: 5px; + display: flex; + flex-wrap: wrap; + align-items: center; + gap: 5px; + } + td > .description { + flex-basis: 100%; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/scss/misc-settings.scss` around lines 65 - 77, The rule td { display: flex } causes .description to sit beside inputs instead of below them; to fix, either change the td flex layout to column (e.g., td { display: flex; flex-direction: column; }) so .description stays under the input, or revert td to normal flow and wrap only the input+icon in a flex container (create a wrapper element/class around the input/icon and apply display:flex to that wrapper) while leaving .description as a block-level sibling so its margin-top: 6px works as intended; update the styles where td and .description are defined accordingly.
🧹 Nitpick comments (4)
includes/admin/pages/connect/steps/pro-credentials.php (1)
14-15: 💤 Low valueRemove redundant initialization.
$is_pro_active = false;on L14 is immediately overwritten on L15 — the assignment never persists. Drop the dead line.♻️ Proposed change
-$is_pro_active = false; $is_pro_active = simcal_is_google_calendar_pro_active(isset($welcome_context) ? (string) $welcome_context : '');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/admin/pages/connect/steps/pro-credentials.php` around lines 14 - 15, Remove the redundant initial assignment to $is_pro_active: the line "$is_pro_active = false;" is immediately overwritten by the call to simcal_is_google_calendar_pro_active(isset($welcome_context) ? (string) $welcome_context : ''), so delete the dead assignment and leave only the call that sets $is_pro_active.includes/admin/pages/connect/partials/helpful-links.php (1)
27-50: ⚡ Quick winApply
rel="noopener noreferrer"consistently to alltarget="_blank"anchors.You added
rel="noopener noreferrer"to the Setup Video link (L39) but the sibling Documentation link (L31) and Support Form link (L45) still open in a new tab without it. Modern browsers default tonoopenerfortarget="_blank", so this is hardening rather than a vulnerability, but consistency makes the intent obvious to reviewers.♻️ Proposed change
<a href="<?php echo simcal_ga_campaign_url( 'https://docs.simplecalendar.io/', 'core-plugin', 'connect-documentation', - ); ?>" target="_blank" class="sc_helpful_link_card"> + ); ?>" target="_blank" rel="noopener noreferrer" class="sc_helpful_link_card"> @@ - <a href="<?php echo esc_url($support_url); ?>" target="_blank" class="sc_helpful_link_card"> + <a href="<?php echo esc_url($support_url); ?>" target="_blank" rel="noopener noreferrer" class="sc_helpful_link_card">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/admin/pages/connect/partials/helpful-links.php` around lines 27 - 50, The Documentation and Support Form anchor tags that call simcal_ga_campaign_url(...) (Documentation link) and echo esc_url($support_url) (Support Form link) both use target="_blank" but are missing rel="noopener noreferrer"; update those two <a> elements to include rel="noopener noreferrer" to match the Setup Video link and ensure consistent hardening for external blank-target links.includes/admin/pages/connect-controller.php (1)
17-20: ⚡ Quick win
$assets_baseon L17 is overwritten byextract()andextract()itself silently overwrites locals.
simcal_prepare_connect_sidebar_scope()returns anassets_basekey (seeincludes/functions/admin.phpL604), so the L17 assignment is shadowed and redundant. More broadly,extract()with the defaultEXTR_OVERWRITEflag silently clobbers any local of the same name, which is fragile as the controller grows. Consider explicit destructuring or at minimumEXTR_SKIPso future additions don't accidentally shadow controller-owned variables.♻️ Suggested adjustment
-$assets_base = SIMPLE_CALENDAR_ASSETS . 'images/admin/'; - $scope = simcal_prepare_connect_sidebar_scope(); -extract($scope); +extract($scope, EXTR_SKIP); +$assets_base = isset($assets_base) ? $assets_base : SIMPLE_CALENDAR_ASSETS . 'images/admin/';Or, if you prefer no
extract()at all, pull only the keys you actually consume:$welcome_context = $scope['welcome_context']; $is_pro_flow = $scope['is_pro_flow']; $api_key = $scope['api_key']; $has_api_key = $scope['has_api_key']; $has_core_api_key_verified = $scope['has_core_api_key_verified']; $has_oauth_connection = $scope['has_oauth_connection']; $has_client_credentials = $scope['has_client_credentials']; $has_published_calendar = $scope['has_published_calendar']; $has_published_pro_calendar= $scope['has_published_pro_calendar']; $should_hide_progress = $scope['should_hide_progress']; $assets_base = $scope['assets_base'];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/admin/pages/connect-controller.php` around lines 17 - 20, The assignment to $assets_base is being overwritten because simcal_prepare_connect_sidebar_scope() returns an assets_base key and extract() (default EXTR_OVERWRITE) silently clobbers locals; replace the fragile extract() usage by either (a) removing the prior $assets_base assignment and calling extract($scope, EXTR_SKIP) to avoid overwriting controller-owned variables, or (b) eliminate extract() entirely and explicitly pull the keys you need from the array returned by simcal_prepare_connect_sidebar_scope() (e.g. assign $welcome_context, $is_pro_flow, $api_key, $has_api_key, $has_core_api_key_verified, $has_oauth_connection, $has_client_credentials, $has_published_calendar, $has_published_pro_calendar, $should_hide_progress, and $assets_base directly from $scope) so nothing is accidentally shadowed.includes/functions/admin.php (1)
386-413: 💤 Low valueSubstring match for fallback Pro detection is a bit permissive.
The fallback scans for
'google-calendar-pro'or'simple-calendar-google-calendar-pro'anywhere in the plugin filename. The latter is canonical, but the former substring could in theory match unrelated plugins whose folder/file names containgoogle-calendar-pro. Since this only runs when none of the runtime signals (constant/classes/is_plugin_active) succeeded — andis_plugin_activewill succeed in the vast majority of installs — the practical exposure is small, but you could tighten the fallback to the exact canonical basename used at L384 to avoid false positives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/functions/admin.php` around lines 386 - 413, The fallback plugin detection is too permissive because it uses strpos on $p_lower for 'google-calendar-pro'; change the logic in the $active_plugins and $sitewide_files loops to test for an exact canonical plugin basename match instead of substring matches — e.g., compare $p_lower with the exact lowercase canonical basename used elsewhere in this file (the same value used by is_plugin_active detection) using strict equality (or strtolower($pro_basename)) rather than strpos, and fall back to the existing substring checks only if that canonical value is not available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/scss/misc-settings.scss`:
- Around line 91-94: The current &:focus rule removes the native outline and
draws a white ring using var(--sc-color-white), which will be low-contrast
against --sc-color-off-white; change the focus styling to use a high-contrast
token (e.g., var(--sc-color-brand) or a new token like --sc-focus-ring) and
prefer :focus-visible to avoid hiding keyboard focus—update the selector from
&:focus to &:focus-visible and replace box-shadow: 0 0 0 2px
var(--sc-color-white) with box-shadow: 0 0 0 2px var(--sc-focus-ring,
var(--sc-color-brand)) so the ring is clearly visible against the off-white
background while keeping the intent of removing the native outline.
---
Duplicate comments:
In `@assets/scss/misc-settings.scss`:
- Around line 65-77: The rule td { display: flex } causes .description to sit
beside inputs instead of below them; to fix, either change the td flex layout to
column (e.g., td { display: flex; flex-direction: column; }) so .description
stays under the input, or revert td to normal flow and wrap only the input+icon
in a flex container (create a wrapper element/class around the input/icon and
apply display:flex to that wrapper) while leaving .description as a block-level
sibling so its margin-top: 6px works as intended; update the styles where td and
.description are defined accordingly.
---
Nitpick comments:
In `@includes/admin/pages/connect-controller.php`:
- Around line 17-20: The assignment to $assets_base is being overwritten because
simcal_prepare_connect_sidebar_scope() returns an assets_base key and extract()
(default EXTR_OVERWRITE) silently clobbers locals; replace the fragile extract()
usage by either (a) removing the prior $assets_base assignment and calling
extract($scope, EXTR_SKIP) to avoid overwriting controller-owned variables, or
(b) eliminate extract() entirely and explicitly pull the keys you need from the
array returned by simcal_prepare_connect_sidebar_scope() (e.g. assign
$welcome_context, $is_pro_flow, $api_key, $has_api_key,
$has_core_api_key_verified, $has_oauth_connection, $has_client_credentials,
$has_published_calendar, $has_published_pro_calendar, $should_hide_progress, and
$assets_base directly from $scope) so nothing is accidentally shadowed.
In `@includes/admin/pages/connect/partials/helpful-links.php`:
- Around line 27-50: The Documentation and Support Form anchor tags that call
simcal_ga_campaign_url(...) (Documentation link) and echo esc_url($support_url)
(Support Form link) both use target="_blank" but are missing rel="noopener
noreferrer"; update those two <a> elements to include rel="noopener noreferrer"
to match the Setup Video link and ensure consistent hardening for external
blank-target links.
In `@includes/admin/pages/connect/steps/pro-credentials.php`:
- Around line 14-15: Remove the redundant initial assignment to $is_pro_active:
the line "$is_pro_active = false;" is immediately overwritten by the call to
simcal_is_google_calendar_pro_active(isset($welcome_context) ? (string)
$welcome_context : ''), so delete the dead assignment and leave only the call
that sets $is_pro_active.
In `@includes/functions/admin.php`:
- Around line 386-413: The fallback plugin detection is too permissive because
it uses strpos on $p_lower for 'google-calendar-pro'; change the logic in the
$active_plugins and $sitewide_files loops to test for an exact canonical plugin
basename match instead of substring matches — e.g., compare $p_lower with the
exact lowercase canonical basename used elsewhere in this file (the same value
used by is_plugin_active detection) using strict equality (or
strtolower($pro_basename)) rather than strpos, and fall back to the existing
substring checks only if that canonical value is not available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b115f6e-6e97-4002-8394-228b42db6e6c
📒 Files selected for processing (7)
assets/scss/misc-settings.scssincludes/admin/ajax.phpincludes/admin/pages/connect-controller.phpincludes/admin/pages/connect/partials/google-calendar-api-key-connect.phpincludes/admin/pages/connect/partials/helpful-links.phpincludes/admin/pages/connect/steps/pro-credentials.phpincludes/functions/admin.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description: Revamp the admin setting make new slug misc_settings and render Calendar and Advance tab on this page.
Summary by CodeRabbit
New Features
Improvements