トップページスライドショーでLargest Contentful Paint の画像のプリロード設定 #1236
Conversation
Walkthroughスライダーの最初の画像をプリロードする処理を追加。コンストラクタで Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
|
@CodeRabbit |
|
@mtdkei さん、承知いたしました。PRのレビューを開始いたします。 ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
_g3/inc/ltg-g3-slider/package/class-ltg-g3-slider.php (2)
22-22: スライダー表示条件のチェックが必要ですこのフックはすべてのページで実行されますが、スライダーはトップページにのみ表示される可能性があります。
preload_first_slide_image()メソッド内で、is_front_page()や$options['top_slide_display']をチェックして、スライダーが実際に表示されるページでのみプリロードタグを出力するようにすることを推奨します。🔎 推奨される修正案
preload_first_slide_image()メソッドの冒頭に条件を追加：public static function preload_first_slide_image() { + // Only preload on pages where the slider is displayed + if ( ! is_front_page() ) { + return; + } + $options = get_option( 'lightning_theme_options' ); $default = function_exists( 'lightning_g3_slider_default_options' ) ? lightning_g3_slider_default_options() : array(); $options = wp_parse_args( $options, $default ); + + // Check if slider is set to display + if ( isset( $options['top_slide_display'] ) && 'hide' === $options['top_slide_display'] ) { + return; + }
867-867: 末尾の空白文字を削除してくださいLine 867の閉じ括弧の後に不要な空白文字（タブまたはスペース）があります。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
_g3/inc/ltg-g3-slider/package/class-ltg-g3-slider.php(3 hunks)readme.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
_g3/inc/ltg-g3-slider/package/class-ltg-g3-slider.php (1)
_g3/inc/ltg-g3-slider/config.php (1)
lightning_g3_slider_default_options(15-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: php unittest (7.4)
- GitHub Check: php unittest (8)
- GitHub Check: php unittest (8.1)
🔇 Additional comments (2)
readme.txt (1)
38-39: LGTM!変更履歴の記載が適切で、追加機能の説明が明確です。
_g3/inc/ltg-g3-slider/package/class-ltg-g3-slider.php (1)
754-757: LGTM!最初のスライド画像に
fetchpriority="high"を適切に追加しており、LCP改善に効果的です。条件分岐も正確です。
| if ( $pc_image === $mobile_image || ( $pc_image && ! $mobile_image ) || ( ! $pc_image && $mobile_image ) ) { | ||
| $img = $pc_image ? $pc_image : $mobile_image; | ||
| echo '<link rel="preload" as="image" href="' . $img . '" fetchpriority="high" />'; | ||
| } else { | ||
| echo '<link rel="preload" as="image" href="' . $mobile_image . '" media="(max-width: 767px)" fetchpriority="high" />'; | ||
| echo '<link rel="preload" as="image" href="' . $pc_image . '" media="(min-width: 768px)" fetchpriority="high" />'; | ||
| } |
There was a problem hiding this comment.
空の画像URLで無効なHTMLが出力される可能性があります
Line 861の条件式に論理的な欠陥があります。$pc_image と $mobile_image の両方が空文字列の場合、$pc_image === $mobile_image が真となり、空の href="" を持つ無効なプリロードタグが出力されます。
🔎 提案される修正
public static function preload_first_slide_image() {
$options = get_option( 'lightning_theme_options' );
$default = function_exists( 'lightning_g3_slider_default_options' ) ? lightning_g3_slider_default_options() : array();
$options = wp_parse_args( $options, $default );
$pc_image = ! empty( $options['top_slide_image_1'] ) ? esc_url( $options['top_slide_image_1'] ) : '';
$mobile_image = ! empty( $options['top_slide_image_mobile_1'] ) ? esc_url( $options['top_slide_image_mobile_1'] ) : '';
- if ( $pc_image === $mobile_image || ( $pc_image && ! $mobile_image ) || ( ! $pc_image && $mobile_image ) ) {
+ // Early return if no images are set
+ if ( ! $pc_image && ! $mobile_image ) {
+ return;
+ }
+
+ if ( $pc_image === $mobile_image || ( $pc_image && ! $mobile_image ) || ( ! $pc_image && $mobile_image ) ) {
$img = $pc_image ? $pc_image : $mobile_image;
- echo '<link rel="preload" as="image" href="' . $img . '" fetchpriority="high" />';
+ echo '<link rel="preload" as="image" href="' . esc_attr( $img ) . '" fetchpriority="high" />' . "\n";
} else {
- echo '<link rel="preload" as="image" href="' . $mobile_image . '" media="(max-width: 767px)" fetchpriority="high" />';
- echo '<link rel="preload" as="image" href="' . $pc_image . '" media="(min-width: 768px)" fetchpriority="high" />';
+ echo '<link rel="preload" as="image" href="' . esc_attr( $mobile_image ) . '" media="(max-width: 767px)" fetchpriority="high" />' . "\n";
+ echo '<link rel="preload" as="image" href="' . esc_attr( $pc_image ) . '" media="(min-width: 768px)" fetchpriority="high" />' . "\n";
- }
+ }
}🤖 Prompt for AI Agents
_g3/inc/ltg-g3-slider/package/class-ltg-g3-slider.php around lines 861-867: the
current condition treats equal empty strings as valid and outputs <link
href="">; change the logic to first ensure at least one non-empty image URL
exists and use strict non-empty checks (e.g., !empty(trim($pc_image)) /
!empty(trim($mobile_image))) before printing preload tags; if both are empty
output nothing, if only one is non-empty preload that one (no media attribute if
only single), and if both non-empty keep the media-specific preload tags as
currently implemented.
|
@CodeRabbit |
|
@mtdkei さん、承知いたしました。PRのレビューを実施いたします。 ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
_g3/inc/ltg-g3-slider/package/class-ltg-g3-slider.php (1)
850-874: 以前のクリティカルな問題が解決されています過去のレビューで指摘された以下の問題が適切に修正されています：
- Line 863の
if ( $img )チェックにより、空のURLによる無効なHTMLの出力を防止- Lines 864, 868, 871で
esc_attr()を使用し、HTML属性のエスケープを実施スライダーの表示設定の確認を推奨
$options['top_slide_display']が'hide'の場合、スライダーは表示されませんが、このメソッドは画像をプリロードします。パフォーマンスの観点から、スライダーが非表示の場合は早期リターンすることを検討してください。🔎 提案される改善
public static function preload_first_slide_image() { $options = get_option( 'lightning_theme_options' ); $default = function_exists( 'lightning_g3_slider_default_options' ) ? lightning_g3_slider_default_options() : array(); $options = wp_parse_args( $options, $default ); + + // Early return if slider is hidden + if ( isset( $options['top_slide_display'] ) && 'hide' === $options['top_slide_display'] ) { + return; + } $pc_image = ! empty( $options['top_slide_image_1'] ) ? esc_url( $options['top_slide_image_1'] ) : ''; $mobile_image = ! empty( $options['top_slide_image_mobile_1'] ) ? esc_url( $options['top_slide_image_mobile_1'] ) : '';行末の空白文字
Line 873に不要な空白文字が含まれています。
🔎 提案される修正
- } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
_g3/inc/ltg-g3-slider/package/class-ltg-g3-slider.php
🧰 Additional context used
🧬 Code graph analysis (1)
_g3/inc/ltg-g3-slider/package/class-ltg-g3-slider.php (1)
_g3/inc/ltg-g3-slider/config.php (1)
lightning_g3_slider_default_options(15-54)
🔇 Additional comments (2)
_g3/inc/ltg-g3-slider/package/class-ltg-g3-slider.php (2)
22-22: wp_headフックへの登録が適切です
preload_first_slide_imageメソッドを優先度1でwp_headに登録することで、プリロードリンクが<head>の早い位置に出力され、LCP改善に効果的です。
754-757: 最初のスライド画像へのfetchpriorityの設定が正確です最初のスライド画像（
$i === 1）のみにfetchpriority="high"属性を付与することで、LCP画像の優先読み込みを実現しています。
|
@CodeRabbit |
|
@mtdkei さん、承知いたしました。PRのレビューを実施いたします。 ✅ Actions performedReview triggered.
|
|
@mtdkei さん、承知いたしました。スライダーが非表示の場合の早期リターン処理を追加されたとのことですね。変更内容を確認してレビューを実施いたします。 ✅ Actions performedReview triggered.
|
|
@mtdkei さん、承知いたしました。最終レビューを実施いたします。 ✅ Actions performedReview triggered.
|
チケットへのリンク / 変更の理由（元のissueがあればリンクを貼り付ければOK）
#660
どういう変更をしたか？
スクリーンショットまたは動画
変更前 Before
変更後 After
実装者の確認事項
実装者はレビュワーに回す前に以下の事を確認してチェックをつけてください。
プログラムの変更の場合
テストを書かないのは普通ではありません。書けるテストは極力書くようにしてください。
変更内容について何を確認したか、どういう方法で確認をしたかなど
レビュワーに回す前の確認事項
レビュワー確認方法・確認内容など
レビュワー向け
レビュワーが確認して変更が反映されていない場合の確認事項
レビューしてみて意図した動作をしない場合は再度ビルドするなど以下の項目を確認してください。
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.