-
Notifications
You must be signed in to change notification settings - Fork 40
fix consistency from index and show routes, add pagination to docs #776
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
this field was present in project_devlogs but not devlogs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR aims to improve API consistency by ensuring that index and show routes return the same fields for their respective resources, and adds pagination documentation for index endpoints that support it.
Key Changes:
- Added
mediafield to devlogs index and show views to return attachment information - Added
vote_count,like_count,devlog_seconds_total, anddevlog_seconds_todayfields to users index view for consistency with the show route - Added pagination documentation to the response schemas for users, projects, devlogs, and project_devlogs index endpoints
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
app/views/api/v1/users/index.jbuilder |
Added vote_count, like_count, devlog_seconds_total, and devlog_seconds_today fields to match show route |
app/views/api/v1/devlogs/show.jbuilder |
Added media field with attachment URLs and content types |
app/views/api/v1/devlogs/index.jbuilder |
Added media field with attachment URLs and content types |
app/controllers/api/v1/users_controller.rb |
Updated response documentation to include new fields and pagination structure |
app/controllers/api/v1/projects_controller.rb |
Added pagination documentation to the index response schema |
app/controllers/api/v1/project_devlogs_controller.rb |
Added pagination documentation to the index response schema |
app/controllers/api/v1/devlogs_controller.rb |
Added media field and pagination documentation to the index response schema |
Comments suppressed due to low confidence (1)
app/controllers/api/v1/devlogs_controller.rb:52
- The
showroute documentation is missing themediafield that is now being returned in the view. This creates an inconsistency between the documented API response and the actual response. Update theshowresponse_body_model to include the media field structure that matches the index route.
show: {
id: Integer,
body: String,
comments_count: Integer,
duration_seconds: Integer,
likes_count: Integer,
scrapbook_url: String,
created_at: String,
updated_at: String
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| json.users @users do |user| | ||
| json.extract! user, :id, :slack_id, :display_name, :avatar | ||
|
|
||
| json.project_ids user.projects.map(&:id) |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistency with the show route: the show route uses @user.projects.pluck(:id) while the index route uses user.projects.map(&:id). For consistency and better performance (since pluck executes at the database level), consider using pluck(:id) here as well.
| json.project_ids user.projects.map(&:id) | |
| json.project_ids user.projects.pluck(:id) |
| @@ -1,5 +1,6 @@ | |||
| json.devlogs @devlogs do |devlog| | |||
| json.extract! devlog, :id, :body, :comments_count, :duration_seconds, :likes_count, :scrapbook_url, :created_at, :updated_at | |||
| json.media devlog.attachments.map { |attachment| { url: Rails.application.routes.url_helpers.rails_blob_path(attachment, only_path: true), content_type: attachment.content_type } } | |||
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause N+1 queries for the attachments association. The controller's index method should eager load attachments using .includes(attachments_attachments: :blob) similar to how ProjectDevlogsController does it to avoid executing a separate query for each devlog's attachments.
Co-authored-by: Copilot <[email protected]>
No description provided.