Merged
Conversation
📝 WalkthroughWalkthroughReactベースの日報一覧をサーバーサイドレンダリングへ移行しました。複数コントローラーで Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant RailsCtr as Rails Controller
participant View as Slim View / Partial
participant DB as Database
Browser->>RailsCtr: GET /users/:id/reports (params: page, practice_id, target?)
RailsCtr->>DB: Report.list joins/where (user/company/practice) + page
DB-->>RailsCtr: records (`@reports`)
RailsCtr->>View: render index with `@reports` (empty? -> empty state : paginated list)
View->>Browser: HTML response (paginated report list or empty state)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/controllers/users/reports_controller.rb`:
- Around line 45-47: The current set_current_user_practice method instantiates
UserCoursePractice with current_user but this view is for another user (`@user`);
change the instantiation in set_current_user_practice to use `@user` (e.g.
UserCoursePractice.new(`@user`)) so the practice dropdown reflects the viewed
user's courses, and add a safe fallback to current_user only if `@user` is nil to
avoid nil errors; update any callers or before_action that rely on
set_current_user_practice accordingly.
🧹 Nitpick comments (2)
app/controllers/users/reports_controller.rb (1)
7-7:set_current_user_practiceは HTML フォーマットでのみ必要この
before_actionは MD ダウンロード時にも実行されますが、@current_user_practiceはビューのフィルターフォームでのみ使用されます。軽微ですが、不要なオブジェクト生成を避けるためにonly: [:index]の制約、またはformat.htmlブロック内での遅延初期化を検討できます。app/views/users/reports/index.html.slim (1)
39-47: 下部のページネーションが.page-content.reportsの外側に配置されていますLine 47 の
= paginate@reports`` は.page-content.reports（Line 40）の兄弟要素になっており、他のビュー（`companies/reports/index.html.slim` や `practices/reports/index.html.slim`）では両方のページネーションが `.page-content.reports` 内に配置されています。`unconfirmed_links_open` パーシャルの配置上の意図かもしれませんが、DOM 構造の一貫性が崩れています。♻️ 一貫性を保つ修正案
.page-content.reports = paginate `@reports` .card-list.a-card .card-list__items = render partial: 'reports/report', collection: `@reports`, as: :report, locals: { user_icon_display: true, actions_display: true } - if mentor_login? && `@target` == 'unchecked_reports' = render partial: 'unconfirmed_links_open', locals: { label: '未チェックの日報を一括で開く' } - = paginate `@reports` + = paginate `@reports`
日報一覧機能をReactからRails viewに移行しました。 - Reports.jsx、Report.jsx、ListComment.jsxを削除 - 5つのviewでreact_componentをRailsパーシャルに置換 - current_user/reports/index - users/reports/index（プラクティスフィルター追加） - practices/reports/index - companies/reports/index - talks/show（サイドバー部分） - 既存の_report.html.slimパーシャルを活用 - kaminariによるページネーション closes #9127 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
set_current_user_practiceで@userを使用するように修正。 閲覧しているユーザーのコースに基づいたプラクティスを ドロップダウンに表示するようにした。 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
komagata
force-pushed
the
feature/de-react-reports
branch
from
4e278a5 to
a56f9b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/current_user/reports/index.html.slim (1)
13-13:⚠️ Potential issue | 🟡 Minor
@user.reports.lengthは全レコードをメモリにロードします。.countまたは.sizeの使用を検討してください。
.lengthは全レコードをRubyの配列としてロードしてからカウントするため、レポート数が多いユーザーではパフォーマンスに影響します。.countであればSQLCOUNTクエリで済みます。♻️ 修正案
- | 自分の日報（#{`@user.reports.length`}） + | 自分の日報（#{`@user.reports.count`}）🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/current_user/reports/index.html.slim` at line 13, The view currently calls `@user.reports.length` which loads all report records into memory; change this to use an ActiveRecord count to avoid loading records—replace `@user.reports.length` with `@user.reports.count` (or .size if you need hybrid behavior) so the template issues a SQL COUNT instead of instantiating all Report objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/users/reports/index.html.slim`:
- Around line 39-47: The bottom paginate call is placed outside the
.page-content.reports container causing inconsistent layout; move the trailing
paginate `@reports` (the call at Line 47) so it sits inside the
.page-content.reports block directly after the conditional render of
'unconfirmed_links_open' (keeping the top paginate, the .card-list rendering of
reports via render partial: 'reports/report', collection: `@reports`, as: :report,
and the mentor_login? && `@target` == 'unchecked_reports' conditional intact) to
match the other views and preserve layout consistency.
---
Outside diff comments:
In `@app/views/current_user/reports/index.html.slim`:
- Line 13: The view currently calls `@user.reports.length` which loads all report
records into memory; change this to use an ActiveRecord count to avoid loading
records—replace `@user.reports.length` with `@user.reports.count` (or .size if you
need hybrid behavior) so the template issues a SQL COUNT instead of
instantiating all Report objects.
Comment on lines
+39
to
+47
| - else | ||
| .page-content.reports | ||
| = paginate @reports | ||
| .card-list.a-card | ||
| .card-list__items | ||
| = render partial: 'reports/report', collection: @reports, as: :report, locals: { user_icon_display: true, actions_display: true } | ||
| - if mentor_login? && @target == 'unchecked_reports' | ||
| = render partial: 'unconfirmed_links_open', locals: { label: '未チェックの日報を一括で開く' } | ||
| = paginate @reports |
There was a problem hiding this comment.
下部のページネーションが .page-content.reports の外側に配置されています。
他のビュー（practices、companies、current_user）では上下両方のページネーションが .page-content.reports 内にありますが、ここでは Line 47 の paginate が .page-content.reports の外に出ています。レイアウトの一貫性が崩れる可能性があります。
♻️ 修正案
- else
.page-content.reports
= paginate `@reports`
.card-list.a-card
.card-list__items
= render partial: 'reports/report', collection: `@reports`, as: :report, locals: { user_icon_display: true, actions_display: true }
- if mentor_login? && `@target` == 'unchecked_reports'
= render partial: 'unconfirmed_links_open', locals: { label: '未チェックの日報を一括で開く' }
- = paginate `@reports`
+ = paginate `@reports`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/users/reports/index.html.slim` around lines 39 - 47, The bottom
paginate call is placed outside the .page-content.reports container causing
inconsistent layout; move the trailing paginate `@reports` (the call at Line 47)
so it sits inside the .page-content.reports block directly after the conditional
render of 'unconfirmed_links_open' (keeping the top paginate, the .card-list
rendering of reports via render partial: 'reports/report', collection: `@reports`,
as: :report, and the mentor_login? && `@target` == 'unchecked_reports' conditional
intact) to match the other views and preserve layout consistency.
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
1 participant
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.
Summary
変更内容
コントローラー
companies/reports_controller.rb-@reportsを追加talks_controller.rb-@reportsを追加（showアクション）users/reports_controller.rb-@current_user_practiceとプラクティスフィルター対応を追加View（5箇所）
current_user/reports/index.html.slimusers/reports/index.html.slimpractices/reports/index.html.slimcompanies/reports/index.html.slimtalks/show.html.slim（サイドバー部分）削除
app/javascript/components/Reports.jsxapp/javascript/components/Report.jsxapp/javascript/components/ListComment.jsxTest plan
bin/rails test test/system/current_user/reports_test.rb- 成功bin/rails test test/system/user/reports_test.rb- 成功bin/rails test test/system/practice/reports_test.rb- 成功bin/rails test test/system/company/reports_test.rb- 成功bin/rails test test/system/talks_test.rb- 成功bin/rails test test/system/reports_practice_filter_test.rb- 成功bin/rails test test/integration/api/reports_test.rb- 成功closes #9127
🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート
新機能
改善
リファクタ