Conversation
kushimegu
force-pushed
the
chore/question_index_vue_to_vanilla_js
branch
4 times, most recently
from
f465bcd to
0ae8a3f
Compare
kushimegu
force-pushed
the
chore/question_index_vue_to_vanilla_js
branch
from
0ae8a3f to
2e80a26
Compare
|
""" Walkthroughこの変更は、質問リストの表示に関する複数のコントローラーおよびビューを修正し、Vue.jsコンポーネントによるクライアントサイド描画から、Railsのサーバーサイドレンダリングによるパーシャル表示へと移行しています。質問の取得やページネーション、ローディングプレースホルダーも追加されています。 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant Model
participant View
User->>Controller: 質問一覧ページにアクセス
Controller->>Model: 質問リストを取得（関連データ・ページネーション含む）
Model-->>Controller: 質問データ返却
Controller->>View: パーシャルを使って質問リストをレンダリング
View-->>User: サーバーサイド描画済みHTMLを返却
Possibly related issues
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.75.5)app/controllers/practices/questions_controller.rbrubocop-minitest extension supports plugin, specify app/controllers/users/questions_controller.rbrubocop-minitest extension supports plugin, specify 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
app/views/questions/_questions.html.slim (1)
1-9: 空リスト表示メッセージが i18n 未対応
empty_messageは controller で組み立てられていますが、ビュー側でハードコードされた日本語文字列を出力しています。
今後の国際化対応を簡単にするためt('questions.empty_message.solved')のような I18n キーを使うことを推奨します。app/views/questions/_question.html.slim (2)
24-26: 冗長な nil チェックすでに
if question.practiceで存在確認が済んでいるため、内側のif !question.practice_id.nil?は不要です。
54-56: 回答数 0 の強調は UX 上分かりにくいかもしれません
answers.empty?のときis-importantを付与していますが、0 件を強調する意図が不明瞭です。
「未回答を目立たせる」目的なら別スタイルやアイコンの方がユーザフレンドリかもしれません。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/controllers/practices/questions_controller.rb(1 hunks)app/controllers/users/questions_controller.rb(1 hunks)app/views/practices/questions/index.html.slim(1 hunks)app/views/questions/_question.html.slim(1 hunks)app/views/questions/_questions.html.slim(1 hunks)app/views/questions/index.html.slim(1 hunks)app/views/shared/_loading_list_placeholder.html.slim(1 hunks)app/views/users/questions/index.html.slim(1 hunks)
🔇 Additional comments (5)
app/views/shared/_loading_list_placeholder.html.slim (1)
1-3: アクセシビリティ向上のためaria-*属性を付与してください
ローディング用スケルトンは視覚的には問題ありませんが、スクリーンリーダーにはリストが 8 件存在するかのように読み上げられます。
以下のようにaria-busy="true"を親要素に、子アイテムにaria-hidden="true"を付与することで、読上げノイズを抑えつつローディング状態を通知できます。-.card-list.a-card.is-loading +.card-list.a-card.is-loading[aria-busy="true"] - - 8.times do + - 8.times do .card-list-item[aria-hidden="true"][ suggest_optional_refactor ]
app/controllers/users/questions_controller.rb (1)
6-10: N+1 クエリ回避用の関連ロードをもう一段深く検討
includes(%i[correct_answer practice answers tag_taggings tags])で主要関連は先読みしていますが、ビュー側でquestion.userやanswers.userを参照している場合は追加読み込みが発生します。必要に応じて下記のように拡張をご検討ください。-@questions = @user.questions.includes(%i[correct_answer practice answers tag_taggings tags]).order(created_at: :desc).page(params[:page]) +@questions = @user.questions + .includes(:user, answers: :user, :correct_answer, :practice, :tag_taggings, :tags) + .order(created_at: :desc) + .page(params[:page])処理量が増える場合は
preloadへの置き換えも検討すると良いです。
[ request_verification ]app/views/questions/index.html.slim (1)
55-56: 部分テンプレート側でのコレクションキャッシュを推奨
render 'questions/questions', questions: @questionsとしているため、ページネーションを跨いで同一質問が再描画されるたびに DB ヒットが起こる可能性があります。_question.html.slim側でcache(question) doを挟むか、ここでrender partial: 'questions/question', collection: @questionsへ変更するとキャッシュヒット率が向上します。
[ suggest_optional_refactor ]app/views/practices/questions/index.html.slim (1)
27-28: 空メッセージローカル変数名の統一を確認
本ビューではempty_message: @empty_message、他ビューでは@questions_property.empty_messageを渡しています。questions/questions部分テンプレートが両方を受け取れる実装になっているか確認してください。
[ request_verification ]app/views/users/questions/index.html.slim (1)
9-10: 無限スクロール導入予定ならdata-controller等の拡張を想定して ID を付与
Vue を除去し DOM が静的になったことで、将来 Stimulus などで無限スクロールを実装する際にフックしやすいよう、ラッパー要素に ID もしくはdata-controller="questions"を付与しておくと保守性が向上します。
[ suggest_nitpick ]
| - question_class = 'is-solved' if question.correct_answer.present? | ||
| - question_class = 'is-wip' if question.wip? |
There was a problem hiding this comment.
🛠️ Refactor suggestion
クラス文字列の上書きにより状態が消える可能性があります
2 行目で question_class = 'is-wip' が上書きされるため、WIP と同時に solved の場合 is-solved が失われます。
配列で管理して join するか、String#<< で追加してください。
-- question_class = 'is-solved' if question.correct_answer.present?
-- question_class = 'is-wip' if question.wip?
+question_classes = []
+question_classes << 'is-solved' if question.correct_answer.present?
+question_classes << 'is-wip' if question.wip?
+question_class = question_classes.join(' ')Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/views/questions/_question.html.slim lines 1 to 2, the assignment to
question_class is overwritten, causing the 'is-solved' class to be lost if both
solved and WIP states are true. Change question_class to an array or use string
concatenation to accumulate both classes, then join or output the combined class
string to preserve all applicable states.
There was a problem hiding this comment.
is-wipとis-solvedのクラス名を同時に使うことはないためclass="#{(question.correct_answer.present? && 'is-solved') || (question.wip? && 'is-wip')}"というコードに変えてquestionに正解がある場合にはis-solved、wipの場合はis-wipというクラス名になるように変更しました
There was a problem hiding this comment.
@kushimegu ありがとうございます！その通りですね。questionが is-wip と is-solved を同時に持つことがないという業務ロジックを理解しました。
あなたの実装 class="#{(question.correct_answer.present? && 'is-solved') || (question.wip? && 'is-wip')}" は、論理OR演算子を使って一つのクラスのみが適用されることを保証する、よりエレガントな解決策ですね。私の配列を使った提案よりもシンプルで適切です。
✏️ Learnings added
Learnt from: kushimegu
PR: fjordllc/bootcamp#8663
File: app/views/questions/_question.html.slim:0-0
Timestamp: 2025-06-30T06:36:32.797Z
Learning: In the bootcamp application, questions cannot be both WIP (work in progress) and solved simultaneously. A question that is still being created (wip?) cannot have a correct answer yet. Therefore, the 'is-wip' and 'is-solved' CSS classes are mutually exclusive.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@sharoa119 |
|
@kushimegu さん レビューの件なのですが、現状引越し片付けがありほとんど作業ができないので 他のメンバーででできそうな方がいたらそちらの方にお願いしていただけたらありがたいのですが、 すぐすぐのお力添えができず、申し訳ないです😫 |
|
@sharoa119 |
|
@matuaya |
|
@kushimegu |
|
@matuaya |
|
@kitarou888 |
|
@kushimegu |
|
@kitarou888 |
|
@kushimegu また、変更確認手順を行いましたが、
ソートの確認方法をご検討いただけるとうれしいです🙇♂️ コードのレビューは明日以降に行いますので、お時間をいただけるとうれしいです！ |
|
@kitarou888 |
|
@kushimegu Issueページで駒形さんが「質問の表示順序を維持すること」と書かれているので、そちらのチェックを kushimegu さんの方でしていただけているのでしたら、私からの動作確認はOKかなと思います。 コードレビューはもう少しお待ちいただければと思います〜。 |
kitarou888
left a comment
kitarou888
left a comment
There was a problem hiding this comment.
@kushimegu
確認が大変遅くなり申し訳ありませんでした。
コードを確認しましたが、特に問題は見られませんでしたので私からは Approve とさせていただきます！
|
@kitarou888 @komagata |
|
@sharoa119 @kushimegu @kitarou888 |
|
@machida @sharoa119 |
|
@kushimegu さん @machida さん この度は私のアカウントがお邪魔をしてしまってすみませんでした🙇♀️ |
|
@sharoa119 @kushimegu @kitarou888 今回はGitHubのバグを踏んだんだと思います。お気になさらず🙏 |
|
@kushimegu CloseしているIssueにすみません。ちょっと質問があるのですが、このPRでは下記のファイルが削除されていないようですがこれはまだ他のどこからか使われている感じでしょうか？
（vue.jsの廃止のためにvueファイルを定期的に検索して、あとどれぐらいで廃止できそうか確認しているためです） |
|
@komagata |
|
@kushimegu そのファイルが現在どうなっているのかの確認と、まだ存在していたら削除のPRを作成いただければありがたいです。 |
|
#9048 にvueファイル削除のPRを作成しました。 |
6 participants
Issue
概要
質問一覧表示からvueを廃止した。
変更確認方法
Screenshot
vue.js廃止のため画面の変更はなし。
質問一覧表示の一例が以下のようになっている。
変更前
変更後
Summary by CodeRabbit
Summary by CodeRabbit
新機能
改善
スタイル