リンクカード：ステージングで特定のリンクカードが展開に失敗している件に対処する#10058
Closed
mousu-a wants to merge 2 commits into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughLinkCheckerとMetadataのエラーハンドリング箇所にRails.loggerによるログ出力を追加。DNS解決失敗、HTTPレスポンス非OK、OpenGraphパース失敗時に関連情報を記録して早期returnするようにした。 Changesエラーハンドリングのロギング強化
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/models/metadata.rb (1)
30-32: ⚡ Quick winログ出力のためだけにHTMLをパースするのはコスト高です。
Nokogiri::HTML(html).titleは、ログメッセージに含めるためだけにHTML全体をパースしています。大きなHTMLドキュメントの場合、このパース処理がパフォーマンスに影響を与える可能性があります。デバッグ目的であれば、タイトル取得を省略するか、HTMLのサイズが小さい場合のみタイトルを取得するなどの最適化を検討してください。
♻️ 最適化案：HTMLサイズチェックを追加
unless object - Rails.logger.info("[Metadata] OpenGraphReader parse failed: url=#{`@url`}, title=#{Nokogiri::HTML(html).title}") + title = html.size < 10_000 ? Nokogiri::HTML(html).title : '(HTML too large to parse)' + Rails.logger.info("[Metadata] OpenGraphReader parse failed: url=#{`@url`}, title=#{title}") return endまたは、よりシンプルにタイトルを省略：
unless object - Rails.logger.info("[Metadata] OpenGraphReader parse failed: url=#{`@url`}, title=#{Nokogiri::HTML(html).title}") + Rails.logger.info("[Metadata] OpenGraphReader parse failed: url=#{`@url`}") return end🤖 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 `@app/models/metadata.rb` around lines 30 - 32, The log currently forces a full HTML parse via Nokogiri::HTML(html).title just to enrich the Rails.logger.info line when object is nil; change this to avoid costly parsing by either omitting the title or gating title extraction behind a size check (e.g., only call Nokogiri::HTML(html).title if html.bytesize is below a threshold), and update the logging in the object-nil branch (the Rails.logger.info call referencing `@url` and Nokogiri::HTML(html).title) to use the safe/conditional approach so we don't parse large documents purely for logging.
🤖 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 `@app/models/metadata.rb`:
- Around line 18-21: The log in the response check (the block around
response.message != 'OK' that calls Rails.logger.info with url=`@url`) currently
writes the full `@url` which may contain sensitive query params; change it to
avoid emitting secrets by either (a) omitting the query string or masking query
values (e.g. parse `@url` with URI and log only scheme+host+path or replace query
values with "[REDACTED]") or (b) only include the full URL in non-production
environments (check Rails.env.production?). Update the Rails.logger.info call to
use a sanitized_url (no query or masked params) or to skip URL logging in
production to prevent leaking tokens/user IDs.
---
Nitpick comments:
In `@app/models/metadata.rb`:
- Around line 30-32: The log currently forces a full HTML parse via
Nokogiri::HTML(html).title just to enrich the Rails.logger.info line when object
is nil; change this to avoid costly parsing by either omitting the title or
gating title extraction behind a size check (e.g., only call
Nokogiri::HTML(html).title if html.bytesize is below a threshold), and update
the logging in the object-nil branch (the Rails.logger.info call referencing
`@url` and Nokogiri::HTML(html).title) to use the safe/conditional approach so we
don't parse large documents purely for logging.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67449a9f-dbd9-4933-9476-303d15217a26
📒 Files selected for processing (2)
app/models/link_checker/checker.rbapp/models/metadata.rb
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/link_checker/checker.rb (1)
42-44: 💤 Low value例外クラスの出力が冗長です。
rescue Resolv::ResolvErrorで既に例外の型を指定しているため、e.classは常にResolv::ResolvErrorになります。ログ出力からe.classを省いても情報の損失はありません。♻️ 簡略化の提案
-rescue Resolv::ResolvError => e - Rails.logger.info("[LinkChecker] #{e.class}: domain=#{domain}") unless Rails.env.production? +rescue Resolv::ResolvError + Rails.logger.info("[LinkChecker] Resolv::ResolvError: domain=#{domain}") unless Rails.env.production? falseまたは例外メッセージを含める場合：
rescue Resolv::ResolvError => e - Rails.logger.info("[LinkChecker] #{e.class}: domain=#{domain}") unless Rails.env.production? + Rails.logger.info("[LinkChecker] Resolv::ResolvError: domain=#{domain}, message=#{e.message}") unless Rails.env.production? 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 `@app/models/link_checker/checker.rb` around lines 42 - 44, In the rescue block that matches "rescue Resolv::ResolvError => e" in app/models/link_checker/checker.rb, remove the redundant e.class from the Rails.logger.info call (since the exception type is already specified) and either log just the domain or, if you want the exception details, include e.message instead; ensure the existing environment guard (unless Rails.env.production?) remains in place.
🤖 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.
Nitpick comments:
In `@app/models/link_checker/checker.rb`:
- Around line 42-44: In the rescue block that matches "rescue
Resolv::ResolvError => e" in app/models/link_checker/checker.rb, remove the
redundant e.class from the Rails.logger.info call (since the exception type is
already specified) and either log just the domain or, if you want the exception
details, include e.message instead; ensure the existing environment guard
(unless Rails.env.production?) remains in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da093c89-a072-4a07-b133-7d1b1f8b1aae
📒 Files selected for processing (2)
app/models/link_checker/checker.rbapp/models/metadata.rb
🚀 Review AppURL: https://bootcamp-pr-10058-fvlfu45apq-an.a.run.app
|
Contributor
Author
|
@komagata ログで気になったところいただいたログに関してですが、以下の文が気になりました。 400が返されていることから、JSからRailsへのfetchは成功しているが、Railsから class API::MetadataController < ApplicationController
...
card = LinkCard::Card.new(params[:url], params[:tweet])
metadata = card.metadata
if metadata
render json: metadata, status: :ok
else
head :bad_request
...考えられる原因、仮説3つほど考えています。
これら3つの仮説を考えました。 |
Member
|
@mousu-a すみません、急ぎだったので僕の方で修正してしまっておりました。 |
Contributor
Author
|
@komagata |
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.
Issue
issueではないですがこちらに端を発しています。
概要
こちらのPRマージ後、ステージングにて動作確認を行ってもらったところリンクカードの展開に失敗していました。
このPRは、ステージングでの問題切り分けのため怪しいところにログ出力を埋め込むものです。
Summary by CodeRabbit
Flakewatch Report