Conversation
reckyy
force-pushed
the
chore/constantize-size-of-ogp-image-in-practice
branch
2 times, most recently
from
1b7567d to
06606fa
Compare
|
@hagiya0121 |
|
@reckyy |
hagiya0121
left a comment
hagiya0121
left a comment
There was a problem hiding this comment.
@reckyy
動作に問題はありませんでした。
気になったところをコメントしたので確認お願いします🙏
| .card-body.is-practice | ||
| - if practice.ogp_image.attached? | ||
| = image_tag practice.ogp_image | ||
| = image_tag practice.ogp_image.variant(resize_to_fill: [1200, 630]).processed.url |
There was a problem hiding this comment.
今の実装でも問題はないと思うのですが、同じメソッドや定数が複数箇所に登場している点が少し気になりました。
個人的には、これをヘルパーにまとめるかモデルのインスタンスメソッドとして定義するのを考えたのですが、どうでしょうか？
There was a problem hiding this comment.
コメントありがとうございます！
自分としても悩んだのですが、このPRの前身であるPRにて、komagataさんから以下のようなコメントがありました。
この「直接表示する」という方針に従い、今回はモデルではなくView側で variant を直接記述する形にしています。
ただご指摘の通り、同じサイズの指定が複数箇所に散らばっている点は保守性の観点から気になります。
View寄りの処理でもあるため、ヘルパーに定数とvariant処理をまとめる案は良いと感じました。
今後の対応としては、komagataさんにも確認のうえ、以下のどちらの方針で進めるか決め、必要があれば修正しようと思います。
- 現状のように
variantを直接Viewに書く形を維持 - 定数と処理をヘルパーに切り出して共通化する形に変更
ご確認よろしくお願いいたします！
There was a problem hiding this comment.
@komagata
現在、PracticeのOGP画像サイズを1200 x 630に統一する修正を行ったのですが、複数箇所に同じサイズ指定が散らばっており、「保守性の観点からヘルパーに共通化してみてはどうか」とメンバーからご提案をいただきました。
ただ、以前の以下のPRで「直接表示する（= Viewでそのままvariantを書く）」というコメントを拝見したのでモデルへの切り出しは避けています。
この場合、以下のどちらの方針が望ましいか、komagataさんのご意見を伺えればと思います。
- 現状のように View に直接
variant(resize_to_fill: [1200, 630])を書く resize_to_fillの定数と処理をヘルパーにまとめる
お手すきの際にご確認いただけますと幸いです。
There was a problem hiding this comment.
@reckyy 2のように共通化して下さい。ただしhelperが最適かどうか実装されたものを見てみないとわからないです。decoratorなども向いているかもしれません。
There was a problem hiding this comment.
@komagata
返信が遅くなり、申し訳ございません。
現在共通化を進めていますので、もうしばらくお待ちいただければと思います。 🙇
|
@reckyy |
reckyy
force-pushed
the
chore/constantize-size-of-ogp-image-in-practice
branch
5 times, most recently
from
82cac7b to
2522ccc
Compare
reckyy
force-pushed
the
chore/constantize-size-of-ogp-image-in-practice
branch
from
2522ccc to
1b755e6
Compare
WalkthroughPracticesHelper に OGP 用の定数 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant View
participant Helper
participant ActiveStorage
participant Router
participant MetaTags
View->>Helper: practice_ogp_image_tag(image)
Helper->>ActiveStorage: variant processing (resize/crop to OGP_SIZE)
ActiveStorage-->>Helper: variant blob
Helper-->>View: rendered image_tag(variant)
View->>Helper: practice_ogp_image_url(image)
Helper->>ActiveStorage: ensure variant (OGP_SIZE)
ActiveStorage-->>Helper: variant blob
Helper->>Router: polymorphic_url(variant)
Router-->>Helper: image_url
View->>Helper: practice_ogp_meta_tags(image_url)
Helper->>MetaTags: set Open Graph & Twitter meta tags (image, url)
MetaTags-->>View: meta tags applied
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.76.1)app/helpers/practices_helper.rbrubocop-minitest extension supports plugin, specify Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/views/practices/unauthorized_show.slim (1)
7-7: 引数の不整合がありますヘルパーメソッドの引数定義と呼び出し方に不整合があります。
practice_ogp_meta_tagsメソッドがキーワード引数を期待するように修正される必要があります。
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/helpers/practices_helper.rb(1 hunks)app/views/mentor/practices/_form.html.slim(1 hunks)app/views/practices/_summary.html.slim(1 hunks)app/views/practices/completion/show.html.slim(1 hunks)app/views/practices/unauthorized_show.slim(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit Configuration File
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/helpers/practices_helper.rb
🧠 Learnings (8)
📓 Common learnings
Learnt from: ryufuta
PR: fjordllc/bootcamp#8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: PRのスコープを維持することが重要であり、バグ修正PRにおいて、修正対象以外の機能（例：ルーティングテスト）の追加提案は適切ではない。そのような改善が必要な場合は別PRで対応すべきである。
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T20:42:19.974Z
Learning: fjordllc/bootcampプロジェクトでは、hタグ（見出し）の文言は日本語でハードコーディングする方針が確立されており、I18n対応は行わない。例：app/views/welcome/logo.html.slimでh2、h3タグが日本語でハードコーディングされている。
📚 Learning: モデルテストにおいて、1つのクラスメソッドの複数の挙動を検証する場合、機能の異なる側面を同じテストメソッドでテストすることは、包括的なテストとして適切である。特に`only_me`のような機能では、異...
Learnt from: kitarou888
PR: fjordllc/bootcamp#8277
File: test/models/searcher_test.rb:326-331
Timestamp: 2025-07-10T12:59:27.807Z
Learning: モデルテストにおいて、1つのクラスメソッドの複数の挙動を検証する場合、機能の異なる側面を同じテストメソッドでテストすることは、包括的なテストとして適切である。特に`only_me`のような機能では、異なる検索条件（空文字と具体的な検索語）を使い分けることで、より広範囲な動作保証が可能となる。
Applied to files:
app/views/practices/_summary.html.slim
📚 Learning: fjordllc/bootcampプロジェクトでは、hタグ（見出し）の文言は日本語でハードコーディングする方針が確立されており、i18n対応は行わない。例：app/views/welcome/logo...
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T20:42:19.974Z
Learning: fjordllc/bootcampプロジェクトでは、hタグ（見出し）の文言は日本語でハードコーディングする方針が確立されており、I18n対応は行わない。例：app/views/welcome/logo.html.slimでh2、h3タグが日本語でハードコーディングされている。
Applied to files:
app/views/practices/_summary.html.slim
📚 Learning: fjordllc/bootcampプロジェクトでは、fixtureファイル内のerb構文で改行制御文字(-)は使用せず、標準的な<% %>と<%= %>のみを使用する方針が統一されている。<% -%>...
Learnt from: su-su-su-su
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-07T05:28:03.676Z
Learning: fjordllc/bootcampプロジェクトでは、fixtureファイル内のERB構文で改行制御文字(-)は使用せず、標準的な<% %>と<%= %>のみを使用する方針が統一されている。<% -%>や<%- %>を使用するとSyntaxErrorが発生する。
Applied to files:
app/views/practices/_summary.html.slim
📚 Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみi18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は...
Learnt from: hirokiej
PR: fjordllc/bootcamp#8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
Applied to files:
app/views/practices/_summary.html.slim
📚 Learning: fjordllc/bootcampプロジェクトでは、rubyの述語メソッドには?を付ける命名規則（例: can_be_checker?）を徹底している。...
Learnt from: su-su-su-su
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-26T15:24:14.799Z
Learning: fjordllc/bootcampプロジェクトでは、Rubyの述語メソッドには?を付ける命名規則（例: can_be_checker?）を徹底している。
Applied to files:
app/views/practices/_summary.html.slim
📚 Learning: prのスコープを維持することが重要であり、バグ修正prにおいて、修正対象以外の機能（例：ルーティングテスト）の追加提案は適切ではない。そのような改善が必要な場合は別prで対応すべきである。...
Learnt from: ryufuta
PR: fjordllc/bootcamp#8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: PRのスコープを維持することが重要であり、バグ修正PRにおいて、修正対象以外の機能（例：ルーティングテスト）の追加提案は適切ではない。そのような改善が必要な場合は別PRで対応すべきである。
Applied to files:
app/views/practices/_summary.html.slim
📚 Learning: このプロジェクトにはrailsの組み込みmarkdownヘルパが利用できないため、markdownのhtml変換には自前でヘルパを実装する必要がある。...
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
Applied to files:
app/views/practices/_summary.html.slimapp/views/mentor/practices/_form.html.slim
⏰ 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_and_test
🔇 Additional comments (3)
app/helpers/practices_helper.rb (1)
4-4: 定数定義が適切ですOGP画像サイズの定数化により、複数箇所での値の重複を解消し、保守性が向上しています。freezeによる不変性の確保も適切です。
app/views/practices/_summary.html.slim (1)
8-8: ヘルパーメソッドへのリファクタリングが適切です直接的な
image_tagの呼び出しをpractice_ogp_image_tagヘルパーメソッドに置き換えることで、OGP画像処理のロジックが一元化され、保守性が向上しています。app/views/practices/unauthorized_show.slim (1)
20-20: ヘルパーメソッドへのリファクタリングが適切です直接的な画像表示処理を
practice_ogp_image_tagヘルパーメソッドに置き換えることで、OGP画像処理のロジックが一元化され、保守性が向上しています。
CodeRabbitの指摘に対応 : #8569 (comment)
CodeRabbitの指摘に対応 : #8569 (comment)
CodeRabbitの指摘に対応 : #8569 (comment)
CodeRabbitの指摘に対応 : #8569 (comment)
reckyy
force-pushed
the
chore/constantize-size-of-ogp-image-in-practice
branch
from
67d0ebd to
728a145
Compare
CodeRabbitの指摘に対応 : #8569 (comment)
CodeRabbitの指摘に対応 : 1b755e6#r2251250267
CodeRabbitの指摘に対応 : #8569 (comment)
reckyy
force-pushed
the
chore/constantize-size-of-ogp-image-in-practice
branch
from
728a145 to
f3bd4ad
Compare
|
@komagata |
| p | ||
| | ここにアップロードした画像が、X（Twitter）、Facebook で投稿した際に表示される画像として使われます。 | ||
| | 画像サイズは必ず 1200px × 630xp でお願いします。 | ||
| | 画像サイズが #{PracticesHelper::OGP_SIZE[0]}px x #{PracticesHelper::OGP_SIZE[1]}px でない場合は、アスペクト比を維持してリサイズし、必要に応じて中央部分を切り取って #{PracticesHelper::OGP_SIZE[0]}px x #{PracticesHelper::OGP_SIZE[1]}px に調整します。 |
|
@reckyy 質問ありがとうございます！！こちら、僕の方で本番で試したいと思います。本番で確認したら連絡しますー |
CodeRabbitの指摘に対応 : #8569 (comment)
CodeRabbitの指摘に対応 : #8569 (comment)
4 participants
Issue
概要
別の方が以下のPRでOGP画像のリサイズを実装されていましたが、新たにメソッドを定義せず、直接OGP画像を表示する方針に変更となったため、新しくPRを作成しました。
※ X（旧Twitter）、FacebookでのOGP画像の表示に関する詳細も、上記PRの概要に記載されています。
変更確認方法
chore/constantize-size-of-ogp-image-in-practiceをローカルに取り込む以降、上記PRと同様の手順をそれぞれ実施してください。
Screenshot
プラクティス編集ページ
プラクティス個別ページ
プラクティス個別ページ（ログアウト状態）
Facebookで投稿時
Twitterで投稿時
Twitterで投稿時（プラクティス修了時）
Summary by CodeRabbit
新機能
ドキュメント
リファクタ