Conversation
There was a problem hiding this comment.
Pull request overview
Adds drag-and-drop ordering for projects within each section and persists the new order to the database.
Changes:
- Added a
PATCH /projects/reorderendpoint to accept reordered IDs per section. - Implemented a
ProjectReorderingServiceto validate and persist section ordering efficiently. - Updated the projects list UI + added a Stimulus sortable controller to enable drag handles and send reorder requests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| config/routes.rb | Adds a collection reorder route for projects. |
| app/controllers/projects_controller.rb | Implements reorder action to call the reordering service. |
| app/services/project_reordering_service.rb | Adds validation + transactional, bulk SQL reordering logic. |
| app/models/project.rb | Changes default ordering to use position and assigns position on create. |
| app/views/projects/_section.html.erb | Adds drag handle column and wires up Stimulus sortable data attributes. |
| app/javascript/controllers/project_sortable_controller.js | Adds Stimulus controller to post new order + section to the server. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div class="project-drag-handle cursor-grab active:cursor-grabbing text-slate-300 hover:text-slate-500 transition"> | ||
| <svg class="h-5 w-5" viewBox="0 0 20 20" fill="currentColor"> | ||
| <path d="M7 2a2 2 0 1 0 .001 4.001A2 2 0 0 0 7 2zm0 6a2 2 0 1 0 .001 4.001A2 2 0 0 0 7 8zm0 6a2 2 0 1 0 .001 4.001A2 2 0 0 0 7 14zm6-8a2 2 0 1 0-.001-4.001A2 2 0 0 0 13 6zm0 2a2 2 0 1 0 .001 4.001A2 2 0 0 0 13 8zm0 6a2 2 0 1 0 .001 4.001A2 2 0 0 0 13 14z" /> | ||
| </svg> | ||
| </div> |
There was a problem hiding this comment.
The drag handle is a plain <div>/<svg> with no accessible name; unlike the todo drag handle elsewhere, this one is missing an aria-label and the SVG lacks aria-hidden. Add an accessible label (or use a <button type="button">) and mark the decorative SVG as aria-hidden to avoid noisy announcements.
| def reorder | ||
| ids = Array(params[:order]).map(&:to_i) | ||
| section = params[:section] | ||
|
|
||
| return head :unprocessable_entity if ids.empty? || section.blank? | ||
|
|
||
| ProjectReorderingService.new(current_user).reorder!( | ||
| ordered_ids: ids, | ||
| section: section | ||
| ) | ||
|
|
||
| head :ok |
There was a problem hiding this comment.
The new ProjectsController#reorder behavior isn’t covered by tests. Add an integration test (similar to existing toggle_complete tests) to assert: 200 on valid reorder, 422 on missing/invalid params, and that positions actually change only for the current user/section.
| def reorder!(ordered_ids:, section:) | ||
| validate_section!(section) | ||
| validate_ordered_ids!(ordered_ids) | ||
|
|
||
| section_sym = section.to_sym | ||
|
|
||
| Project.transaction do | ||
| section_projects = @user.projects.active | ||
| .where(section: section_sym) | ||
| .lock("FOR UPDATE") | ||
|
|
||
| section_ids = section_projects.pluck(:id) | ||
|
|
||
| validate_ids_match_section!(section_ids, ordered_ids) | ||
|
|
||
| reorder_projects_in_section(section_projects, ordered_ids) | ||
| end |
There was a problem hiding this comment.
ProjectReorderingService introduces non-trivial validation and update logic but has no test coverage. Add a test/services/project_reordering_service_test.rb modeled after test/services/todo_reordering_service_test.rb to cover successful reorder, invalid section, invalid/duplicate IDs, missing/extra IDs, and ensuring other sections/users aren’t affected.
| ) | ||
|
|
||
| head :ok | ||
| rescue ProjectReorderingService::Error => e |
There was a problem hiding this comment.
rescue ProjectReorderingService::Error => e assigns e but never uses it, which will be flagged by RuboCop (and also drops useful debugging context). Either remove the exception variable (rescue ProjectReorderingService::Error) or log e.message similarly to TodosController#reorder.
| rescue ProjectReorderingService::Error => e | |
| rescue ProjectReorderingService::Error => e | |
| Rails.logger.error("Project reordering failed: #{e.message}") |
| <table class="w-full"> | ||
| <thead> | ||
| <tr class="border-b border-slate-200 text-left text-sm font-medium text-slate-500"> | ||
| <th class="pb-2 w-8"></th> |
There was a problem hiding this comment.
The new drag-handle column header is an empty <th>, which is not accessible for screen readers. Add a visually-hidden label (e.g., “Reorder”) or appropriate ARIA so the column has a meaningful header.
| <th class="pb-2 w-8"></th> | |
| <th class="pb-2 w-8" scope="col" aria-label="Reorder"></th> |
1 participant
No description provided.