Conversation
Walkthroughテスト用のアバタリセットヘルパー Changes
Sequence Diagram(s)sequenceDiagram
participant Test as System Test
participant Helper as AvatarHelper
participant AS as ActiveStorage
participant User as User model
Test->>Helper: reset_avatar(user)
Helper->>AS: open fixture file and build blob params
Helper->>AS: create_or_find blob by custom key
alt blob exists
AS-->>Helper: existing blob returned
Helper-->>User: ensure user avatar attached (no upload)
else blob created
Helper->>AS: upload blob data
Helper->>User: attach new blob to user
end
Test->>Test: 実際のテスト操作
sequenceDiagram
participant UserModel as User#attach_custom_avatar
participant AS as ActiveStorage
UserModel->>AS: create_or_find blob by custom key
alt blob exists
UserModel-->>UserModel: 既存Blobを利用（アップロード省略）
else blob created
UserModel->>AS: upload data to new blob
UserModel->>AS: attach blob to user
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分 Possibly related issues
Possibly related PRs
Suggested reviewers
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/models/user.rbrubocop-minitest extension supports plugin, specify ✨ 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 comments)
Other keywords and placeholders
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: 0
🧹 Nitpick comments (1)
test/supports/avatar_helper.rb (1)
3-13: 実装は適切ですが、エラーハンドリングの追加を検討してください。AvatarHelper の実装は正しく、ActiveStorage の
attachメソッドを適切に使用しています。ただし、テストフィクスチャファイルが存在しない場合のエラーハンドリングを追加することで、より堅牢になります。以下の diff でエラーハンドリングを追加できます：
def reset_avatar(user) filename = "#{user.login_name}.jpg" path = Rails.root.join("test/fixtures/files/users/avatars", filename) + + unless File.exist?(path) + raise "Avatar fixture file not found: #{path}" + end + user.avatar.attach( io: File.open(path, "rb"), filename:, content_type: 'image/jpeg' ) end
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
test/application_system_test_case.rb(2 hunks)test/supports/avatar_helper.rb(1 hunks)test/system/admin/users_test.rb(1 hunks)test/system/attachments_test.rb(1 hunks)test/system/searchables_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
test/system/admin/users_test.rbtest/system/searchables_test.rbtest/application_system_test_case.rbtest/system/attachments_test.rbtest/supports/avatar_helper.rb
test/**/*
⚙️ CodeRabbit Configuration File
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/system/admin/users_test.rbtest/system/searchables_test.rbtest/application_system_test_case.rbtest/system/attachments_test.rbtest/supports/avatar_helper.rb
🧠 Learnings (4)
📓 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: ryufuta
PR: fjordllc/bootcamp#8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: Railsの標準的なヘルパーメソッド（例：`question_url`、`user_path`など）が通常の方法で使用されている場合、それらの動作テストは不要である。これらのメソッドはRailsフレームワーク自体でテストされており、アプリケーション固有のロジックではないため。
📚 Learning: 2025-07-10T12:59:27.807Z
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:
test/system/searchables_test.rbtest/system/attachments_test.rb
📚 Learning: 2025-07-30T07:26:36.599Z
Learnt from: ryufuta
PR: fjordllc/bootcamp#8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: Railsの標準的なヘルパーメソッド（例：`question_url`、`user_path`など）が通常の方法で使用されている場合、それらの動作テストは不要である。これらのメソッドはRailsフレームワーク自体でテストされており、アプリケーション固有のロジックではないため。
Applied to files:
test/application_system_test_case.rbtest/system/attachments_test.rb
📚 Learning: 2025-06-21T22:30:20.116Z
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:
test/application_system_test_case.rb
🧬 Code Graph Analysis (4)
test/system/admin/users_test.rb (1)
test/supports/avatar_helper.rb (1)
reset_avatar(4-12)
test/system/searchables_test.rb (1)
test/supports/avatar_helper.rb (1)
reset_avatar(4-12)
test/application_system_test_case.rb (1)
test/test_helper.rb (2)
include(22-55)include(57-60)
test/system/attachments_test.rb (1)
test/supports/avatar_helper.rb (1)
reset_avatar(4-12)
⏰ 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 (4)
test/system/admin/users_test.rb (1)
100-102: 適切な修正とコメントです。webpテストでのアバターリセットの追加は適切で、なぜこの処理が必要かを明確に説明するコメントも含まれています。テスト環境でのアバター共有による画像変換の問題を解決する良いアプローチです。
test/system/searchables_test.rb (1)
189-191: 一貫した実装で適切です。他のシステムテストと同様のパターンでアバターリセットを実装しており、コメントも統一されています。webp形式の確認テストに必要な前準備として適切です。
test/system/attachments_test.rb (1)
8-14: 適切な修正とコードの改善です。アバターリセットの追加に加えて、ローカル変数
userの導入によりコードの可読性が向上しています。他のシステムテストと一貫したパターンで実装されており、適切です。test/application_system_test_case.rb (1)
15-15: AvatarHelper の統合が適切に実装されています。新しい
AvatarHelperモジュールの require と include が正しく追加されており、すべてのシステムテストでreset_avatarメソッドが利用できるようになっています。Also applies to: 29-29
Ryooo-k
force-pushed
the
chore/fix-flaky-webp-avatar-test
branch
from
c971949 to
e29d2a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
app/models/user.rb (1)
707-713:avatar_urlの判定が不十分—既存Blobがあっても未attachだとwebp化をスキップ
ActiveStorage::Blob.find_by(key: ...)の存在で呼び出し可否を決めると、「Blobはあるがユーザーのavatarが未付け替え」の場合にattach_custom_avatarが呼ばれず、非webpのURLを返してしまいます。現在のユーザーが参照しているBlobのkeyを見て判断してください。- attach_custom_avatar if !ActiveStorage::Blob.find_by(key: "avatars/#{login_name}.#{AVATAR_FORMAT}") + target_key = "avatars/#{login_name}.#{AVATAR_FORMAT}" + attach_custom_avatar if avatar.blob.key != target_key任意でさらに厳格にするなら content_type も併せて判定可:
attach_custom_avatar if avatar.blob.key != target_key || avatar.blob.content_type != "image/#{AVATAR_FORMAT}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/models/user.rb(1 hunks)test/application_system_test_case.rb(2 hunks)test/models/user_test.rb(2 hunks)test/supports/avatar_helper.rb(1 hunks)test/system/admin/users_test.rb(1 hunks)test/system/attachments_test.rb(1 hunks)test/system/searchables_test.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/application_system_test_case.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- test/system/admin/users_test.rb
- test/supports/avatar_helper.rb
- test/system/searchables_test.rb
- test/system/attachments_test.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
test/models/user_test.rbapp/models/user.rb
test/**/*
⚙️ CodeRabbit Configuration File
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/user_test.rb
🧠 Learnings (2)
📓 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: ryufuta
PR: fjordllc/bootcamp#8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: Railsの標準的なヘルパーメソッド（例：`question_url`、`user_path`など）が通常の方法で使用されている場合、それらの動作テストは不要である。これらのメソッドはRailsフレームワーク自体でテストされており、アプリケーション固有のロジックではないため。
📚 Learning: 2025-07-10T12:59:27.807Z
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:
test/models/user_test.rb
⏰ 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
Ryooo-k
force-pushed
the
chore/fix-flaky-webp-avatar-test
branch
from
1efdb5c to
ae2efd5
Compare
|
@komagata |
app/models/user.rb
Outdated
| ) | ||
| avatar.attach(custom_blob) | ||
|
|
||
| with_lock do |
There was a problem hiding this comment.
すみません、これってこうすることでどういう問題をどうやって防ぐ感じですかね？
There was a problem hiding this comment.
意図
同一ユーザーでavatar_urlが並列に呼ばれた際、key重複による一意制約違反を防ぐため、with_lockで悲観的ロックを取り、ロック内で再チェックしています。
起きていた事象
- A・Bが並列で
avatar_urlを実行 avatar_urlのkeyチェックでは、どちらも「custom_keyのBlobはない」と判定- A・Bとも
attach_custom_avatarを実行 - Aがcustom_avatarを作成 → 直後にBも同じkeyで作成しようとする
- Bでは既にAで作成されたcustom_avatarが存在するので、一意制約違反が発生
この事象はtest/system/notifications_test.rbで発生しました。
対策
- with_lockで同一ユーザーへのDB更新を直列化
- ロック後にkeyを再チェック
- 無ければ作成、あればreturn
There was a problem hiding this comment.
@Ryooo-k なるほどです。説明ありがとうございます。
with_lockは悲観的ロックなのでパフォーマンスがすごい下がる可能性がありそうでできれば使いたくないなと思います。
ActiveRecordのtransactionを使うやり方とかcreate_or_find_byなどで対応できないでしょうか？
There was a problem hiding this comment.
create_or_find_byを使用する形で修正いたしました！
96d82d3 ご確認お願いします。
test/models/user_test.rb
Outdated
| assert_equal '/images/users/avatars/default.png', user_with_default_avatar.avatar_url | ||
|
|
||
| user_with_custom_avatar = users(:komagata) | ||
| # ユーザーアイコンがwebpに変換されていることを確認するテストは、対象となるavatarをresetする。 |
There was a problem hiding this comment.
この説明はreset_avatarの方にメソッドの説明コメントとして書いた方がいいかなと思いました。
|
@komagata 一部、修正がございましたので、改めてご連絡いたします。申し訳ありません🙇 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/storage.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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: ryufuta
PR: fjordllc/bootcamp#8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: Railsの標準的なヘルパーメソッド（例：`question_url`、`user_path`など）が通常の方法で使用されている場合、それらの動作テストは不要である。これらのメソッドはRailsフレームワーク自体でテストされており、アプリケーション固有のロジックではないため。
⏰ 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
| test: | ||
| service: Disk | ||
| root: <%= ENV['ACTIVE_STORAGE_TEST_ROOT'] || Rails.root.join("tmp/storage") %> | ||
| root: <%= Rails.root.join("tmp/storage") %> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
並列テスト時の衝突リスク：test用ストレージrootの固定化によりワーカー間で干渉する可能性があります
tmp/storage に固定すると、並列実行（プロセス/スレッド）時に同一ディレクトリ配下で書き込み/削除が競合し、まれにフレークの再発や意図せぬパージが起き得ます。以前のような環境変数によるオーバーライドを残しておくと、必要に応じてワーカー毎に分離したrootを指定でき、安定性確保の逃げ道になります（デフォルトは現状の固定パスのままにできます）。
後方互換を保ちつつの修正案（推奨）:
test:
service: Disk
- root: <%= Rails.root.join("tmp/storage") %>
+ root: <%= ENV["ACTIVE_STORAGE_TEST_ROOT"].presence || Rails.root.join("tmp/storage") %>
public: true補足:
- 本PRでテストの安定化（avatarリセット、attach_custom_avatarのwith_lock）は進んでいますが、ストレージrootの固定は将来的な並列度変更やCI環境差分で再度フレーク要因になり得ます。envオーバーライドを残すだけならスコープを広げずにリスク低減できます。
並列実行状況と既存参照の有無を確認するため、以下をリポジトリルートで実行してください（sandboxでクローン済み前提）:
🏁 Script executed:
#!/bin/bash
set -eu
echo "== ActiveStorage test root の既存利用確認 =="
rg -n 'ACTIVE_STORAGE_TEST_ROOT' -S || true
echo "== Railsの並列化設定の確認（minitest/rails） =="
rg -n 'parallelize\(' -A 3 test || true
rg -n 'Parallelization' -S test || true
echo "== parallel_tests系の導入痕跡（TEST_ENV_NUMBERなど） =="
rg -n 'TEST_ENV_NUMBER' -S || true
rg -n 'parallel_tests' -S || trueLength of output: 699
テスト用ストレージrootに環境変数オーバーライドを復活し、並列実行の衝突を回避
test/test_helper.rbでparallelize(workers: :number_of_processors)が有効化されているため、全ワーカーで同一のtmp/storageを共有すると書き込み/削除が競合し、テストがフレークする恐れがあります。以下のように環境変数ACTIVE_STORAGE_TEST_ROOTを残しつつ、デフォルトで各ワーカーごとに別フォルダを参照する設定にすると安全です。
修正箇所:
- config/storage.yml の
test.root
提案差分:
test:
service: Disk
- root: <%= Rails.root.join("tmp/storage") %>
+ root: <%= ENV["ACTIVE_STORAGE_TEST_ROOT"].presence ||
+ Rails.root.join("tmp/storage#{ENV['TEST_ENV_NUMBER']}") %>
public: true必要に応じてCIやローカルの並列テスト設定でACTIVE_STORAGE_TEST_ROOTをワーカーごとに設定してください。
📝 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.
| root: <%= Rails.root.join("tmp/storage") %> | |
| test: | |
| service: Disk | |
| root: <%= ENV["ACTIVE_STORAGE_TEST_ROOT"].presence || | |
| Rails.root.join("tmp/storage#{ENV['TEST_ENV_NUMBER']}") %> | |
| public: true |
🤖 Prompt for AI Agents
In config/storage.yml at line 3, the test storage root is currently hardcoded to
tmp/storage, which causes conflicts during parallel test execution. Modify the
root setting to use the environment variable ACTIVE_STORAGE_TEST_ROOT if set,
falling back to tmp/storage with a unique subdirectory per worker to avoid
collisions. This ensures each parallel test worker uses a separate storage path,
preventing write/delete conflicts.
There was a problem hiding this comment.
@coderabbitai
こちらは、test_helper.rbにて以下のように設定しているため、root設定は現状のままでOKです。
parallelize_setup do |i|
ActiveStorage::Blob.service.instance_variable_set(:@root, "#{ActiveStorage::Blob.service.root}/storage-#{i}")
ActiveStorage::Blob.services.fetch(:test_fixtures).instance_variable_set(:@root, "#{ActiveStorage::Blob.services.fetch(:test_fixtures).root}/fixtures-#{i}")
end
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the parallelize_setup configuration in test_helper.rb
echo "=== Checking test_helper.rb for parallelize_setup configuration ==="
cat test/test_helper.rb | grep -A 5 -B 2 "parallelize_setup" || echo "parallelize_setup not found in test/test_helper.rb"
# Also check if there are any other parallelization-related configurations
echo -e "\n=== Checking for other parallelization configurations ==="
rg "parallelize" test/test_helper.rb -A 3 -B 1
Length of output: 2039
@Ryooo-k なるほど、確認いたしました。test_helper.rbでparallelize_setupを使用して、各ワーカーごとにstorage-#{i}のサフィックスを付けてディレクトリを分離しているため、並列実行時の衝突は適切に回避されていますね。
parallelize_setup do |i|
ActiveStorage::Blob.service.instance_variable_set(:@root, "#{ActiveStorage::Blob.service.root}/storage-#{i}")
ActiveStorage::Blob.services.fetch(:test_fixtures).instance_variable_set(:@root, "#{ActiveStorage::Blob.services.fetch(:test_fixtures).root}/fixtures-#{i}")
endこの実装により、storage.ymlでの環境変数による設定は不要となり、シンプルな固定パスで問題ないことを理解しました。ご説明ありがとうございます。
🧠 Learnings used
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: ryufuta
PR: fjordllc/bootcamp#8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: Railsの標準的なヘルパーメソッド（例：`question_url`、`user_path`など）が通常の方法で使用されている場合、それらの動作テストは不要である。これらのメソッドはRailsフレームワーク自体でテストされており、アプリケーション固有のロジックではないため。
|
@komagata |
app/models/user.rb
Outdated
| content_type: "image/#{AVATAR_FORMAT}", | ||
| identify: false | ||
| ) | ||
| created = false |
There was a problem hiding this comment.
createdという変数を用意しているのが不要な気がしました。
custom_blob.id_previously_was.present?メソッドでcreateされたのかどうか確かめられた気がします。
There was a problem hiding this comment.
31112c6 にて修正しました🙇
便利なメソッドがたくさんあるんですね、、、勉強になります！
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/user.rb(1 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/models/user.rb
🧠 Learnings (1)
📚 Learning: 2025-07-23T20:31:13.856Z
Learnt from: jun-kondo
PR: fjordllc/bootcamp#8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトの`app/controllers/reports_controller.rb`において、`create`と`update`アクションは両方とも`report.save_uniquely`を使用し、同じ`:report_save`イベントと`'report.save'`イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
Applied to files:
app/models/user.rb
⏰ 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
| custom_key = "avatars/#{login_name}.#{AVATAR_FORMAT}" | ||
| variant_avatar = avatar.variant(resize_to_fill: AVATAR_SIZE, autorot: true, saver: { strip: true, quality: 60 }, format: AVATAR_FORMAT).processed | ||
| io = StringIO.new(variant_avatar.download) | ||
| custom_blob = ActiveStorage::Blob.create_and_upload!( | ||
| io:, | ||
| key: "avatars/#{login_name}.#{AVATAR_FORMAT}", | ||
| filename: "#{login_name}.#{AVATAR_FORMAT}", | ||
| content_type: "image/#{AVATAR_FORMAT}", | ||
| identify: false | ||
| ) | ||
| custom_blob = ActiveStorage::Blob.create_or_find_by!(key: custom_key) do |blob| | ||
| blob.filename = "#{login_name}.#{AVATAR_FORMAT}" | ||
| blob.content_type = "image/#{AVATAR_FORMAT}" | ||
| blob.byte_size = io.size | ||
| blob.checksum = Digest::MD5.base64digest(io.read) | ||
| io.rewind | ||
| end | ||
| return if custom_blob.id_previously_was.present? | ||
|
|
||
| custom_blob.upload(io, identify: false) | ||
| avatar.attach(custom_blob) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
既存Blob未attach・無駄なvariant生成・並列時の不安定さを解消しましょう
現状の問題点:
- 既存Blobが存在してもユーザーのavatarへ付け替えずにreturnするため、staleなattachmentを参照し続けるケースが残ります（webp化されない・フレークの原因）。
- create_or_find_by! 実行前に毎回 variant 生成・download しており、既存Blobがある場合でも不要な画像処理が走ります。
- 並列実行で先行が作成し、後続が find パスに入った場合の挙動が早期returnに寄りすぎで、attach漏れや不要uploadのリスクがあります。
以下の修正で改善してください:
- 先に key で find し、存在すれば即 attach（variantは生成しない）。
- 無ければ variant 生成→ create_or_find_by!。既存返却なら upload はせず attach のみ。新規作成時のみ upload→attach。
- avatar_url 側の呼び出し条件も「Blobが存在しない」ではなく「現在のattachmentのkeyが custom_key と異なる」場合に変更。
適用差分（該当範囲のみ）:
- custom_key = "avatars/#{login_name}.#{AVATAR_FORMAT}"
- variant_avatar = avatar.variant(resize_to_fill: AVATAR_SIZE, autorot: true, saver: { strip: true, quality: 60 }, format: AVATAR_FORMAT).processed
- io = StringIO.new(variant_avatar.download)
- custom_blob = ActiveStorage::Blob.create_or_find_by!(key: custom_key) do |blob|
- blob.filename = "#{login_name}.#{AVATAR_FORMAT}"
- blob.content_type = "image/#{AVATAR_FORMAT}"
- blob.byte_size = io.size
- blob.checksum = Digest::MD5.base64digest(io.read)
- io.rewind
- end
- return if custom_blob.id_previously_was.present?
-
- custom_blob.upload(io, identify: false)
- avatar.attach(custom_blob)
+ custom_key = "avatars/#{login_name}.#{AVATAR_FORMAT}"
+ # 既存Blobがあれば付け替えて終了（variant生成は不要）
+ if (existing = ActiveStorage::Blob.find_by(key: custom_key))
+ avatar.attach(existing) unless avatar.blob_id == existing.id
+ return
+ end
+
+ # ここに到達したら初回生成（またはレースで直前に他プロセスが作成した可能性）
+ variant_avatar = avatar.variant(
+ resize_to_fill: AVATAR_SIZE,
+ autorot: true,
+ saver: { strip: true, quality: 60 },
+ format: AVATAR_FORMAT
+ ).processed
+ io = StringIO.new(variant_avatar.download)
+
+ custom_blob = ActiveStorage::Blob.create_or_find_by!(key: custom_key) do |blob|
+ blob.filename = "#{login_name}.#{AVATAR_FORMAT}"
+ blob.content_type = "image/#{AVATAR_FORMAT}"
+ blob.byte_size = io.size
+ blob.checksum = Digest::MD5.base64digest(io.string)
+ end
+
+ # レースで既存が返ってきた場合はuploadせずに付け替えのみ
+ if custom_blob.id_previously_was.present?
+ avatar.attach(custom_blob) unless avatar.blob_id == custom_blob.id
+ return
+ end
+
+ custom_blob.upload(io, identify: false)
+ avatar.attach(custom_blob)avatar_url 側も合わせて以下のように変更することで、既存Blobがあっても「現在のattachmentがcustom_keyでない場合」は確実に付け替えられます（参考コード・別箇所のため差分外）:
def avatar_url
if avatar.attached? && avatar.blob.present?
custom_key = "avatars/#{login_name}.#{AVATAR_FORMAT}"
attach_custom_avatar unless avatar.blob.key == custom_key
"#{avatar.url}?v=#{avatar.created_at.to_i}"
else
image_url DEFAULT_IMAGE_PATH
end
rescue ActiveStorage::FileNotFoundError, ActiveStorage::InvariableError => e
log_avatar_error('avatar_url', e)
image_url DEFAULT_IMAGE_PATH
endこの変更で:
- 既存Blobがあるときは O(1) attach、無駄な変換ダウンロードを回避
- レース時も「attach漏れ」や「不要upload」を避けつつ一貫して custom_key に収束
- webp期待のテストのフレーク要因を除去
必要であれば該当テストの修正提案や追加の回帰テストも用意します。
🤖 Prompt for AI Agents
In app/models/user.rb around lines 956 to 969, avoid generating the image
variant and downloading it before checking for an existing blob; instead first
try to find an ActiveStorage::Blob by the key
("avatars/#{login_name}.#{AVATAR_FORMAT}") and if found immediately attach it to
the avatar (no variant work), otherwise generate the variant, build the blob via
create_or_find_by! and then: if create_or_find_by! returns an existing blob
simply attach it (do not upload), but if it returns a newly created blob upload
the IO and then attach; also remove the early return that skips attaching when
id_previously_was is present so attach always occurs when the current attachment
key differs; finally update avatar_url caller logic (separate method) to trigger
attach_custom_avatar when avatar.blob.key != custom_key instead of only when
blob is missing.
There was a problem hiding this comment.
@coderabbitai
問題点に関して、回答します。
既存Blobが存在してもユーザーのavatarへ付け替えずにreturnするため、staleなattachmentを参照し続けるケースが残ります（webp化されない・フレークの原因）。
「既存Blobがcustom_keyであればwebpへの変換は行われている」ということになるため、その場合は付け替え不要でり、returnで返すことは問題ないと思います。
create_or_find_by! 実行前に毎回 variant 生成・download しており、既存Blobがある場合でも不要な画像処理が走ります。
avatar_urlメソッドでcustom_keyのチェックは行っていますので、create_or_find_by! 実行前に毎回 variant 生成・download を行っても問題ないと考えています。
並列実行で先行が作成し、後続が find パスに入った場合の挙動が早期returnに寄りすぎで、attach漏れや不要uploadのリスクがあります。
findされた場合は、custom_keyが存在していることになり、custom_keyであればwebpに変換されているはずなので、その場合は、attachやuploadは不要です。
There was a problem hiding this comment.
@Ryooo-k ご説明ありがとうございます。実装の意図を理解しました。
確かに以下の点でロジックは一貫していますね：
- custom_keyのBlobが存在 = 既にwebp変換済み
- avatar_urlでcustom_keyの存在確認を行い、存在しない場合のみ変換処理を実行
- 並列実行時もcreate_or_find_by!で適切にハンドリング
パフォーマンス面では既存Blob存在時のvariant生成を避ける余地はありますが、機能的には問題なく動作することを理解しました。テストが安定して通るようになったという結果も、この実装が適切に機能していることを示していますね。
私の最初の提案は過度に最適化を意識したものでした。現在の実装で問題ないと判断されたことを承知しました。
（＾・ω・＾）
|
@komagata |
|
ご確認ありがとうございます！ |
2 participants
Issue
概要
ユーザーアイコンがwebpになっていることを期待するテストがFlakyとなっていましたので、改善をいたしました。
改善内容
ユーザー画像リセットによる安定化
webp関連のテストメソッド内で、ユーザー画像をリセットする処理を導入し、テストを安定させました。
（複数テストで画像変換処理が行われることにより、元データが削除されテストがFlakyとなっていたため）
key重複による一意制約エラーを回避
create_and_upload!メソッドでは、並列処理時にkey重複が起こる場合があり、一意制約エラーが発生してました。create_or_find_by!メソッドに変更し、keyが重複していればcreateしないようにし、一意制約エラーを回避しました。変更確認方法
下記テストが問題ないことを確認する。
rails test test/system/admin/users_test.rb ci=1rails test test/system/searchables_test.rb ci=1rails test test/system/attachments_test.rb ci=1rails test test/models/user_test.rb ci=1rails test test/system/notifications_test.rb ci=1RuntimeError: Neutered Exception ActionView::Template::Errorでテストが失敗しないことを確認する。rails test test/system/current_user_test.rb ci=1ActionView::Template::Errorでテストが失敗しないことを確認する。Summary by CodeRabbit