Phase 1 MVP機能を完成（Users・Subscription・Skip・RSS・Post前後ナビ）#4
Conversation
## 実装内容 ### 1. UsersController とユーザーページ - `app/controllers/users_controller.rb` を作成 - `app/views/users/show.html.slim` を作成 - `/@username` でユーザープロフィールと参加中スレッド一覧を表示 ### 2. Subscription 機能（購読/購読解除） - `app/controllers/subscriptions_controller.rb` を作成 - Thread 詳細ページに購読ボタンを追加（is_subscribing 変数で状態管理） - トップページで購読中スレッドを優先表示（bg-blue-50 で視覚的に区別） - ログイン時は「購読中のスレッド」と「その他のスレッド」を分離表示 ### 3. Skip 機能（ターンスキップ） - `app/controllers/skips_controller.rb` を作成 - Skip モデルに `after_create :update_thread_turn` コールバックを追加 - Thread 詳細ページに「今回はパス」ボタンを追加（確認ダイアログ付き） - スキップすると次のメンバーにターンが移る ### 4. RSS フィード - ThreadsController#show に `respond_to` ブロックを追加 - `app/views/threads/show.rss.builder` を作成（最新10件の投稿を配信） - Thread 詳細ページに RSS リンクを追加 ### 5. Post 前後ナビゲーション - Post モデルに `prev` / `next` メソッドを追加 - Post 詳細ページに前後の投稿へのナビゲーションリンクを追加 これで Phase 1 MVP の全機能が完成しました。 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @sugiwe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! このプルリクエストは、Phase 1 MVPの残りの5つの主要機能を実装し、プロジェクトの最初のマイルストーンを達成しました。これにより、ユーザーは自分のプロフィールページを持ち、興味のあるスレッドを購読してトップページで優先的に表示させることができ、また、自分のターンをスキップしてスムーズなコミュニケーションを促進し、スレッドの更新をRSSフィードで追跡し、投稿間を簡単に移動できるようになります。これらの機能は、ユーザーエクスペリエンスを向上させ、プラットフォームの基本的なインタラクティブ性を確立します。 Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements multiple key features for the MVP. However, it introduces significant security vulnerabilities related to broken access control and information exposure. Specifically, the new user profile page leaks the existence and slugs of private threads, and the thread detail page (including the new RSS feed) does not enforce visibility restrictions, allowing unauthorized access to potentially sensitive content. These security issues must be addressed before merging. Additionally, to further enhance code quality and maintainability, I've suggested improvements such as optimizing queries in threads_controller, fixing a bug in the post model's navigation methods, and utilizing partials to reduce code duplication in views.
app/controllers/users_controller.rb
Outdated
|
|
||
| def show | ||
| @user = User.find_by!(username: params[:username]) | ||
| @threads = @user.correspondence_threads.includes(:users, :memberships).order(last_posted_at: :desc, created_at: :desc) |
There was a problem hiding this comment.
The show action in UsersController retrieves all threads where the user is a member without filtering for visibility. This causes private threads (e.g., those with visibility set to url_only, followers_only, or paid) to be listed on the user's public profile page. An attacker can discover the existence and slugs of these private threads, which may contain sensitive information or were intended to be hidden.
To remediate this, filter the threads by visibility in the controller. Only public threads should be shown to the general public.
@threads = @user.correspondence_threads.where(visibility: "public").includes(:users, :memberships).order(last_posted_at: :desc, created_at: :desc)| respond_to do |format| | ||
| format.html | ||
| format.rss { render layout: false } | ||
| end |
There was a problem hiding this comment.
The show action in ThreadsController lacks visibility checks, and it is exempt from authentication via skip_before_action :require_login. This allows anyone, including unauthenticated users, to view the content of any thread (via HTML or the newly added RSS feed) if they know the slug. This bypasses the intended restrictions for followers_only and paid threads.
To remediate this, implement a check in the show action or a before_action filter to ensure the current user has permission to view the thread based on its visibility setting. For example:
def show
unless @thread.visibility == "public" || @thread.visibility == "url_only" || @thread.memberships.exists?(user: current_user)
redirect_to root_path, alert: "アクセス権限がありません"
return
end
# ... existing logic
end| def prev | ||
| thread.posts.where("created_at < ?", created_at).order(created_at: :desc).first | ||
| end | ||
|
|
||
| def next | ||
| thread.posts.where("created_at > ?", created_at).order(created_at: :asc).first | ||
| end |
There was a problem hiding this comment.
default_scope で order(created_at: :asc) が設定されているため、prev メソッドの order(created_at: :desc) が正しく機能しません。order はソート条件を追加するだけで、default_scope のソートを上書きしないため、ORDER BY created_at ASC, created_at DESC のようなSQLが生成されてしまいます。default_scope を上書きするには reorder を使う必要があります。
また、next メソッドでは default_scope のソート順がそのまま使えるため、order の呼び出しは不要です。
def prev
thread.posts.where("created_at < ?", created_at).reorder(created_at: :desc).first
end
def next
thread.posts.where("created_at > ?", created_at).first
end|
|
||
| if logged_in? | ||
| @subscribed_threads = current_user.subscribed_threads.merge(base_query).order(last_posted_at: :desc, created_at: :desc) | ||
| @other_threads = base_query.where.not(id: @subscribed_threads.pluck(:id)).order(last_posted_at: :desc, created_at: :desc) |
There was a problem hiding this comment.
pluck を使うと、購読中のスレッドIDを取得するためのクエリが余分に発生してしまいます。Active Recordリレーションを直接 where.not に渡すことで、より効率的なサブクエリを生成できます。これにより、データベースへのラウンドトリップを1回減らすことができます。
@other_threads = base_query.where.not(id: current_user.subscribed_threads).order(last_posted_at: :desc, created_at: :desc)
app/views/threads/index.html.slim
Outdated
| - @subscribed_threads.each do |thread| | ||
| = link_to thread_path(thread.slug), class: "block border border-gray-200 rounded-lg p-5 hover:border-gray-400 transition-colors bg-blue-50" do | ||
| .flex.items-start.justify-between.gap-4 | ||
| .flex-1.min-w-0 | ||
| h3.text-base.font-medium.truncate = thread.title | ||
| - if thread.description.present? | ||
| p.text-sm.text-gray-500.mt-1.line-clamp-2 = thread.description | ||
| .flex.items-center.gap-3.mt-3.text-xs.text-gray-400 | ||
| span = thread.users.map { |u| "@#{u.username}" }.join(" × ") | ||
| - if thread.last_posted_at | ||
| span · | ||
| span = time_ago_in_words(thread.last_posted_at) + "前" | ||
|
|
||
| / その他のスレッド | ||
| - if @other_threads.present? | ||
| h2.text-base.font-medium.text-gray-700.mb-3 = @subscribed_threads.present? ? "その他のスレッド" : "すべてのスレッド" | ||
| .flex.flex-col.gap-4 | ||
| - @other_threads.each do |thread| | ||
| = link_to thread_path(thread.slug), class: "block border border-gray-200 rounded-lg p-5 hover:border-gray-400 transition-colors" do | ||
| .flex.items-start.justify-between.gap-4 | ||
| .flex-1.min-w-0 | ||
| h3.text-base.font-medium.truncate = thread.title | ||
| - if thread.description.present? | ||
| p.text-sm.text-gray-500.mt-1.line-clamp-2 = thread.description | ||
| .flex.items-center.gap-3.mt-3.text-xs.text-gray-400 | ||
| span = thread.users.map { |u| "@#{u.username}" }.join(" × ") | ||
| - if thread.last_posted_at | ||
| span · | ||
| span = time_ago_in_words(thread.last_posted_at) + "前" |
There was a problem hiding this comment.
購読中スレッドとその他のスレッドを表示するロジックがほぼ同じで、コードが重複しています。この部分はパーシャル（例: app/views/threads/_thread.html.slim）に切り出すことで、コードの重複をなくし、見通しを良くすることができます。
パーシャルに切り出すことで、app/views/users/show.html.slim でも再利用できるようになり、保守性が向上します。
例えば、以下のようにパーシャルを呼び出せます。
/ app/views/threads/index.html.slim
/ ...
- if @subscribed_threads.present?
/ ...
= render "threads/thread", collection: @subscribed_threads, as: :thread, locals: { highlight: true }
/ ...
- if @other_threads.present?
/ ...
= render "threads/thread", collection: @other_threads, as: :thread/ app/views/threads/_thread.html.slim
- highlight_class = local_assigns.fetch(:highlight, false) ? "bg-blue-50" : ""
= link_to thread_path(thread.slug), class: "block border border-gray-200 rounded-lg p-5 hover:border-gray-400 transition-colors #{highlight_class}" do
.flex.items-start.justify-between.gap-4
.flex-1.min-w-0
h3.text-base.font-medium.truncate = thread.title
- if thread.description.present?
p.text-sm.text-gray-500.mt-1.line-clamp-2 = thread.description
.flex.items-center.gap-3.mt-3.text-xs.text-gray-400
span = thread.users.map { |u| "@#{u.username}" }.join(" × ")
- if thread.last_posted_at
span ·
span = time_ago_in_words(thread.last_posted_at) + "前"
app/views/users/show.html.slim
Outdated
| - @threads.each do |thread| | ||
| = link_to thread_path(thread.slug), class: "block border border-gray-200 rounded-lg p-5 hover:border-gray-400 transition-colors" do | ||
| .flex.items-start.justify-between.gap-4 | ||
| .flex-1.min-w-0 | ||
| h3.text-base.font-medium.truncate = thread.title | ||
| - if thread.description.present? | ||
| p.text-sm.text-gray-500.mt-1.line-clamp-2 = thread.description | ||
| .flex.items-center.gap-3.mt-3.text-xs.text-gray-400 | ||
| span = thread.users.map { |u| "@#{u.username}" }.join(" × ") | ||
| - if thread.last_posted_at | ||
| span · | ||
| span = time_ago_in_words(thread.last_posted_at) + "前" |
There was a problem hiding this comment.
ここのスレッド一覧表示部分は app/views/threads/index.html.slim と同じコードの重複が見られます。別のコメントで提案したようにスレッド表示をパーシャルに切り出すことで、こちらでも再利用でき、コードの保守性が向上します。
## 問題 ユーザーページで visibility フィルタリングがされておらず、 将来的に非公開スレッドの存在が漏洩するリスクがあった。 ## 修正内容 - `@user.correspondence_threads` に `.where(visibility: "public")` を追加 - Phase 3 で非公開スレッド機能を追加した際の情報漏洩を防止 ## 影響 - 現時点では全スレッドが public なので動作に変化なし - 将来の visibility 拡張時にセキュリティホールを防げる 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## 問題 ThreadsController#show が visibility チェックなしで誰でもアクセス可能だった。 将来的に followers_only や paid スレッドを追加した際にセキュリティリスクとなる。 ## 修正内容 - `can_view_thread?` helper メソッドを追加 - Phase 1 では `visibility == "public"` のみ閲覧可能 - Phase 3 での拡張方法をコメントで明記 - HTML と RSS の両方に権限チェックを適用 ## 設計 - public: 誰でも閲覧可能（Phase 1 で実装済み） - url_only: URL を知っていれば閲覧可能（Phase 3 で追加予定） - followers_only / paid: メンバーのみ閲覧可能（Phase 3 で追加予定） 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## 問題 default_scope で `order(created_at: :asc)` が設定されているため、 prev メソッドの `.order(created_at: :desc)` が正しく機能していなかった。 Rails の `.order()` は既存のソート条件に追加されるため、 `ORDER BY created_at ASC, created_at DESC` という重複したSQLが生成され、 ASC が優先されて意図と逆の結果になっていた。 ## 修正内容 - `prev` メソッド: `.order()` → `.reorder()` に変更 - default_scope を上書きして正しく直前の投稿を取得 - `next` メソッド: `.order(created_at: :asc)` を削除 - default_scope をそのまま利用するため不要 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## 問題 `pluck(:id)` を使うと2回のクエリが発行されていた: 1. SELECT id FROM threads WHERE ... (pluck で ID 配列を取得) 2. SELECT * FROM threads WHERE id NOT IN (1, 2, 3, ...) (除外クエリ) ## 修正内容 `.pluck(:id)` を削除し、Active Record リレーションを直接渡すことで サブクエリとして1回のSQLで実行されるように最適化: SELECT * FROM threads WHERE id NOT IN (SELECT thread_id FROM subscriptions WHERE ...) ## メリット - データベースへのラウンドトリップが2回→1回に削減 - メモリ効率が向上（Ruby 側で ID 配列を保持しない） - より効率的な実行計画をデータベースが立てられる 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## 問題 threads/index と users/show でスレッド表示のコードが重複していた。 購読中スレッドとその他のスレッドでも同じコードを繰り返していた。 ## 修正内容 - `app/views/threads/_thread.html.slim` パーシャルを作成 - `highlight` パラメータで購読中スレッドの青背景を制御 - threads/index.html.slim で3箇所の重複を削減 - users/show.html.slim でもパーシャルを再利用 ## メリット - DRY原則に従い、コードの重複を削減 - 保守性向上：修正が1箇所で済む - 可読性向上：メインビューがシンプルに 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
このプルリクエストは、MVPのフェーズ1を完成させるための5つの主要な機能（ユーザーページ、購読、スキップ、RSS、投稿ナビゲーション）を実装するもので、多岐にわたる変更が含まれています。全体として、機能は適切に実装されており、コードもクリーンです。特に、_thread.html.slimパーシャルを導入してビューの重複を削減した点は素晴らしい改善です。
いくつか、コードの保守性やパフォーマンスをさらに向上させるための提案をさせていただきます。具体的には、コントローラー間のロジックの共通化、ビューでのデータベースクエリの回避、そしてより明確なコード表現に関するものです。詳細は各コメントをご確認ください。
app/views/posts/show.html.slim
Outdated
| - if @post.prev | ||
| = link_to thread_post_path(@thread.slug, @post.prev), class: "flex items-center gap-2 text-sm text-gray-500 hover:text-gray-700" do | ||
| span ← | ||
| .flex.flex-col.items-start | ||
| span.text-xs 前の投稿 | ||
| span.font-medium = truncate(@post.prev.title, length: 30) | ||
| - else | ||
| .w-32 | ||
| - if @post.next | ||
| = link_to thread_post_path(@thread.slug, @post.next), class: "flex items-center gap-2 text-sm text-gray-500 hover:text-gray-700" do | ||
| .flex.flex-col.items-end | ||
| span.text-xs 次の投稿 | ||
| span.font-medium = truncate(@post.next.title, length: 30) | ||
| span → | ||
| - else | ||
| .w-32 |
There was a problem hiding this comment.
ビューファイル内で@post.prevや@post.nextを呼び出すと、ページのレンダリング中にデータベースクエリが実行されてしまいます。これはパフォーマンスの低下に繋がる可能性があり、MVCの責務分離の観点からも望ましくありません。さらに、@post.prevが複数回呼び出されており、非効率です。
これらのロジックはコントローラに移動し、結果をインスタンス変数（例: @prev_post, @next_post）としてビューに渡すことを強く推奨します。
- if @prev_post
= link_to thread_post_path(@thread.slug, @prev_post), class: "flex items-center gap-2 text-sm text-gray-500 hover:text-gray-700" do
span ←
.flex.flex-col.items-start
span.text-xs 前の投稿
span.font-medium = truncate(@prev_post.title, length: 30)
- else
.w-32
- if @next_post
= link_to thread_post_path(@thread.slug, @next_post), class: "flex items-center gap-2 text-sm text-gray-500 hover:text-gray-700" do
.flex.flex-col.items-end
span.text-xs 次の投稿
span.font-medium = truncate(@next_post.title, length: 30)
span →
- else
.w-32
| def set_thread | ||
| @thread = CorrespondenceThread.find_by!(slug: params[:thread_slug]) | ||
| rescue ActiveRecord::RecordNotFound | ||
| redirect_to root_path, alert: "スレッドが見つかりません" | ||
| end |
There was a problem hiding this comment.
set_threadメソッドがSubscriptionsControllerにも同様に実装されており、コードが重複しています。このような共通ロジックはConcernに切り出すことで、コードの重複をなくし、保守性を向上させることができます。
| if logged_in? | ||
| @subscribed_threads = current_user.subscribed_threads.merge(base_query).order(last_posted_at: :desc, created_at: :desc) | ||
| @other_threads = base_query.where.not(id: @subscribed_threads).order(last_posted_at: :desc, created_at: :desc) | ||
| else | ||
| @threads = base_query.order(last_posted_at: :desc, created_at: :desc) | ||
| end |
There was a problem hiding this comment.
ソート順を定義するorder(last_posted_at: :desc, created_at: :desc)が3箇所で繰り返されています。これをローカル変数や定数として定義することで、コードの可読性が向上し、将来的な変更が容易になります。
order_hash = { last_posted_at: :desc, created_at: :desc }
if logged_in?
@subscribed_threads = current_user.subscribed_threads.merge(base_query).order(order_hash)
@other_threads = base_query.where.not(id: @subscribed_threads).order(order_hash)
else
@threads = base_query.order(order_hash)
end
app/models/post.rb
Outdated
| end | ||
|
|
||
| def next | ||
| thread.posts.where("created_at > ?", created_at).first |
There was a problem hiding this comment.
default_scopeにorderが指定されているため、このfirstは意図通りに動作します。しかし、default_scopeへの依存はコードの可読性を下げ、予期せぬ挙動を招く可能性があります。prevメソッドのようにreorderを使って明示的にソート順を指定するか、orderを明記することをお勧めします。
thread.posts.where("created_at > ?", created_at).reorder(created_at: :asc).first
app/views/threads/_thread.html.slim
Outdated
| @@ -0,0 +1,12 @@ | |||
| - highlight_class = local_assigns[:highlight] ? "bg-blue-50" : "" | |||
There was a problem hiding this comment.
local_assigns を使ってローカル変数の存在を確認するのは、Railsの内部実装に依存しており、可読性が高い方法とは言えません。defined? を使うか、変数が常に渡されることが保証されている場合は直接参照する方が、よりクリーンで意図が明確になります。
- highlight_class = (defined?(highlight) && highlight) ? "bg-blue-50" : ""
## 問題 ビューで `@post.prev` と `@post.next` を呼び出すと、ページレンダリング中に データベースクエリが実行され、さらに同じメソッドが複数回呼ばれていた。 これはパフォーマンスの低下とMVCの責務分離の観点から望ましくない。 ## 修正内容 - PostsController#show で `@prev_post` と `@next_post` を事前取得 - ビューでインスタンス変数を使用するよう変更 ## メリット - パフォーマンス向上：クエリが1回のみ実行される保証 - 可読性向上：ビューがシンプルに - MVC の責務が明確に：コントローラでデータ準備、ビューで表示のみ - テスタビリティ向上：コントローラで事前にデータが確定 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Gemini のコードレビュー指摘により、set_thread と require_membership の 重複を解消するため、当初 Concern を使用する案で進めたが、構造の明確性を 優先し、名前空間を使った継承アプローチに変更。 変更内容: - app/controllers/threads/application_controller.rb を作成し、共通メソッド set_thread と require_membership を定義 - 以下のコントローラーを threads/ ディレクトリに移動し、 Threads::ApplicationController を継承するよう変更: - PostsController → Threads::PostsController - SubscriptionsController → Threads::SubscriptionsController - SkipsController → Threads::SkipsController - InvitationsController → Threads::InvitationsController - 対応するビューファイルも app/views/threads/ 配下に移動 - routes.rb でコントローラー指定を更新 (controller: "threads/...") - アラートメッセージを「このスレッドのメンバーではありません」に統一 この構造により、「スレッドに紐づくリソース」という概念が明確になり、 将来的な拡張時にも分かりやすい設計となった。 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Gemini のコードレビュー指摘により、 `order(last_posted_at: :desc, created_at: :desc)` が ThreadsController と UsersController で重複していたため、 CorrespondenceThread モデルに `recent_order` スコープを追加。 変更内容: - CorrespondenceThread に scope :recent_order を追加 - ThreadsController#index で .recent_order を使用 - UsersController#show で .recent_order を使用 これにより、ソート順のロジックが一箇所にまとまり、 将来的な変更が容易になった。 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Gemini のコードレビュー指摘により、next メソッドが default_scope に 暗黙的に依存していたため、明示的に reorder(created_at: :asc) を追加。 変更内容: - Post#next メソッドに .reorder(created_at: :asc) を追加 - prev メソッドと同様に、default_scope に依存しない実装に統一 これにより、コードの可読性が向上し、予期せぬ挙動を防ぐことができる。 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Gemini のコードレビュー指摘により、local_assigns を使った 間接参照から、直接参照に変更してコードをシンプル化。 変更内容: - threads/_thread.html.slim で local_assigns[:highlight] を 直接 highlight に変更 - すべての呼び出し元で highlight 変数が渡されることが 保証されているため、安全に直接参照できる - より読みやすく、意図が明確なコードになった 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
| skip_before_action :set_thread | ||
| before_action :require_membership, only: [ :create ] | ||
| before_action :set_thread_for_create, only: [ :create ] |
There was a problem hiding this comment.
require_membership は @thread が設定されていることを前提としていますが、この順序では @thread を設定する set_thread_for_create よりも先に実行されるため、@thread が nil となりエラーになります。また、親クラスの set_thread を活用することで実装を簡略化できます。
skip_before_action :set_thread, only: [ :show, :accept ]
before_action :require_membership, only: [ :create ]| def show | ||
| @prev_post = @post.prev | ||
| @next_post = @post.next | ||
| end |
There was a problem hiding this comment.
スレッドの visibility が public 以外の場合でも、URLを知っていれば誰でも投稿を閲覧できてしまう可能性があります。ThreadsController#show と同様のアクセス権限チェックを導入することを検討してください。
| def can_view_thread?(thread) | ||
| # Phase 1: public のみ閲覧可能 | ||
| return true if thread.visibility == "public" | ||
|
|
||
| # Phase 3 で追加予定の visibility: | ||
| # - url_only: URL を知っていれば誰でも閲覧可能 | ||
| # return true if thread.visibility == "url_only" | ||
| # | ||
| # - followers_only / paid: メンバーのみ閲覧可能 | ||
| # return true if logged_in? && thread.memberships.exists?(user: current_user) | ||
|
|
||
| false | ||
| end |
There was a problem hiding this comment.
スレッドのメンバーであれば、visibility が public でなくても閲覧できるべきです。現状の実装では、メンバーであっても非公開スレッド（将来的に追加される url_only など）にアクセスできなくなる可能性があります。
def can_view_thread?(thread)
return true if thread.visibility == "public"
return true if logged_in? && thread.memberships.exists?(user: current_user)
false
end| unless @thread.memberships.exists?(user: current_user) | ||
| redirect_to thread_path(@thread.slug), alert: "このスレッドのメンバーではありません" | ||
| end | ||
| end |
There was a problem hiding this comment.
PostsController と SkipsController で重複している require_my_turn メソッドをここに定義することで、共通化できます。
end
def require_my_turn
unless @thread.my_turn?(current_user)
redirect_to thread_path(@thread.slug), alert: "今はあなたのターンではありません"
end
end| def set_thread_for_create | ||
| @thread = CorrespondenceThread.find_by!(slug: params[:thread_slug]) | ||
| rescue ActiveRecord::RecordNotFound | ||
| redirect_to root_path, alert: "スレッドが見つかりません" | ||
| end |
There was a problem hiding this comment.
親クラスの set_thread で代用可能なため、このメソッドは不要です。
| def require_my_turn | ||
| unless @thread.my_turn?(current_user) | ||
| redirect_to thread_path(@thread.slug), alert: "今はあなたのターンではありません" |
There was a problem hiding this comment.
親クラス Threads::ApplicationController にメソッドを移動したため、この定義は削除可能です。
| def require_my_turn | ||
| unless @thread.my_turn?(current_user) | ||
| redirect_to thread_path(@thread.slug), alert: "今はあなたのターンではありません" | ||
| end | ||
| end |
There was a problem hiding this comment.
親クラス Threads::ApplicationController にメソッドを移動したため、この定義は削除可能です。
| def prev | ||
| thread.posts.where("created_at < ?", created_at).reorder(created_at: :desc).first | ||
| end | ||
|
|
||
| def next | ||
| thread.posts.where("created_at > ?", created_at).reorder(created_at: :asc).first | ||
| end |
There was a problem hiding this comment.
created_at が同一の投稿が存在する場合、順序が不安定になる可能性があります。id をタイブレーカーとして使用することで、より確実に前後の投稿を取得できます。
def prev
thread.posts.where("created_at < ? OR (created_at = ? AND id < ?)", created_at, created_at, id).reorder(created_at: :desc, id: :desc).first
end
def next
thread.posts.where("created_at > ? OR (created_at = ? AND id > ?)", created_at, created_at, id).reorder(created_at: :asc, id: :asc).first
endThere was a problem hiding this comment.
created_at が同一ってのは可能性はゼロではないけど、実質的には考えなくて良いかなと思いました。
app/models/skip.rb
Outdated
| after_create :update_thread_turn | ||
|
|
||
| # Scopes | ||
| default_scope -> { order(created_at: :desc) } |
There was a problem hiding this comment.
default_scope は予期せぬクエリへの影響を及ぼす可能性があるため、通常の scope または明示的な order の使用が推奨されます。
scope :recent, -> { order(created_at: :desc) }Gemini のコードレビュー指摘により、before_action の実行順序の 問題を修正。require_membership が実行される時点で @thread が nil だったため、親クラスの set_thread を活用するように変更。 変更内容: - skip_before_action :set_thread を全アクションではなく show と accept のみに限定 - create アクションでは親の set_thread が params[:thread_slug] から @thread を設定するため、require_membership が正しく動作 - set_thread_for_create メソッドを削除（重複のため不要） これにより、コードが簡潔になり、親クラスの機能を 適切に再利用できるようになった。 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Gemini のコードレビュー指摘により、PostsController と SkipsController で重複していた require_my_turn メソッドを 親クラスに移動して共通化。 変更内容: - Threads::ApplicationController に require_my_turn を追加 - PostsController から require_my_turn を削除 - SkipsController から require_my_turn を削除（private セクションも不要に） これにより、ターン制チェックのロジックが一箇所に集約され、 set_thread や require_membership と同様に、Threads 名前空間内の すべてのコントローラーで利用可能になった。 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Gemini のコードレビュー指摘により、default_scope を通常の scope に変更してベストプラクティスに準拠。 変更内容: - default_scope を scope :recent に変更 - 現時点で Skip のレコードを取得する処理は存在しないため、 既存コードへの影響なし - 将来的に Skip レコードを取得する際は、明示的に .recent を 呼ぶことで意図が明確になる default_scope は予期せぬクエリへの影響を及ぼす可能性があり、 Rails コミュニティでは非推奨とされているため、この変更により コードの保守性が向上する。 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
| @subscribed_threads = current_user.subscribed_threads.merge(base_query) | ||
| @other_threads = base_query.where.not(id: @subscribed_threads) |
There was a problem hiding this comment.
where.not(id: <ActiveRecord::Relation>) はサブクエリを生成します。この方法は動作しますが、pluck を使ってIDの配列を先に取得し、それを where と where.not に渡す方が、より明示的で、多くの場合パフォーマンスも向上します。現在の実装では、@subscribed_threads のリレーションが評価されるたびにクエリが実行される可能性があり、非効率になることがあります。
subscribed_thread_ids = current_user.subscribed_thread_ids
@subscribed_threads = base_query.where(id: subscribed_thread_ids)
@other_threads = base_query.where.not(id: subscribed_thread_ids)| def prev | ||
| thread.posts.where("created_at < ?", created_at).reorder(created_at: :desc).first | ||
| end | ||
|
|
||
| def next | ||
| thread.posts.where("created_at > ?", created_at).reorder(created_at: :asc).first | ||
| end |
There was a problem hiding this comment.
created_at のタイムスタンプが完全に同一である場合、現在の順序付けは非決定的になる可能性があります。これにより、prev/next メソッドが予期しない投稿を返したり、投稿をスキップしたりするバグが発生する可能性があります。id をセカンダリソートキーとして追加することで、順序を完全に決定的にし、この問題を回避できます。
def prev
thread.posts.where("created_at < ?", created_at).reorder(created_at: :desc, id: :desc).first
end
def next
thread.posts.where("created_at > ?", created_at).reorder(created_at: :asc, id: :asc).first
end1 participant
概要
Phase 1 MVP の残り5機能を実装し、完成させました。
実装内容
1. UsersController とユーザーページ (15分)
ファイル:
app/controllers/users_controller.rbapp/views/users/show.html.slim機能:
/@usernameでユーザープロフィールを表示2. Subscription 機能（購読/購読解除） (30分)
ファイル:
app/controllers/subscriptions_controller.rbapp/views/threads/show.html.slim(購読ボタン追加)app/views/threads/index.html.slim(購読中スレッド優先表示)app/controllers/threads_controller.rb(index アクション更新)機能:
bg-blue-50) で視覚的に区別命名:
is_subscribing変数を使用（能動的な購読状態を表現）3. Skip 機能（ターンスキップ） (20分)
ファイル:
app/controllers/skips_controller.rbapp/models/skip.rb(コールバック追加)app/views/threads/show.html.slim(スキップボタン追加)機能:
after_createコールバックでthread.last_post_user_idを更新4. RSS フィード (20分)
ファイル:
app/controllers/threads_controller.rb(respond_to追加)app/views/threads/show.rss.builderapp/views/threads/show.html.slim(RSS リンク追加)機能:
/:slug.rssで個別スレッドの RSS 2.0 フィードを配信5. Post 前後ナビゲーション (15分)
ファイル:
app/models/post.rb(prev/nextメソッド追加)app/views/posts/show.html.slim(ナビゲーション UI 追加)機能:
prev/nextメソッドを実装テスト項目
/@username) が正しく表示される/:slug.rss) が正しく配信されるPhase 1 完成
これで Phase 1 MVP の全機能が実装完了しました！🎉
次のステップ:
🤖 Generated with Claude Code