Add drag-to-reorder on daily work queue

ldraney commented

2026-05-25 01:53:34 +00:00

Owner

Summary

Drag-to-reorder queue items via Sortable.js + Stimulus controller
Bulk position update endpoint in a single transaction
6-dot grip drag handle with grab cursor and ghost styling

Changes

config/importmap.rb: pin sortablejs@1.15.6 from jspm CDN
app/javascript/controllers/sortable_controller.js: Stimulus controller wrapping Sortable.js, sends reordered IDs via PATCH
app/controllers/work_queue_items_controller.rb: reorder action with bulk position update in transaction
config/routes.rb: patch :reorder collection route on work_queue_items
app/views/work_queue_items/_queue_item.html.erb: drag handle SVG before each item
app/views/work_queue_items/index.html.erb: wire sortable controller on queue list
app/assets/stylesheets/application.css: drag handle + .is-dragging ghost styles
spec/requests/work_queue_items_spec.rb: 2 new specs for bulk and single-item reorder

Test Plan

Request spec: bulk reorder updates positions correctly
Request spec: single-item reorder works
All 17 existing specs pass

Review Checklist

No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #7
project-landscaping-assistant

## Summary - Drag-to-reorder queue items via Sortable.js + Stimulus controller - Bulk position update endpoint in a single transaction - 6-dot grip drag handle with grab cursor and ghost styling ## Changes - `config/importmap.rb`: pin sortablejs@1.15.6 from jspm CDN - `app/javascript/controllers/sortable_controller.js`: Stimulus controller wrapping Sortable.js, sends reordered IDs via PATCH - `app/controllers/work_queue_items_controller.rb`: `reorder` action with bulk position update in transaction - `config/routes.rb`: `patch :reorder` collection route on work_queue_items - `app/views/work_queue_items/_queue_item.html.erb`: drag handle SVG before each item - `app/views/work_queue_items/index.html.erb`: wire sortable controller on queue list - `app/assets/stylesheets/application.css`: drag handle + `.is-dragging` ghost styles - `spec/requests/work_queue_items_spec.rb`: 2 new specs for bulk and single-item reorder ## Test Plan - [x] Request spec: bulk reorder updates positions correctly - [x] Request spec: single-item reorder works - [x] All 17 existing specs pass ## Review Checklist - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - Closes #7 - `project-landscaping-assistant`

ldraney added 1 commit

2026-05-25 01:53:34 +00:00

Add drag-to-reorder on daily work queue

ci/woodpecker/push/woodpecker Pipeline was successful

Details

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

CI / scan_ruby (pull_request) Has been cancelled

Details

CI / scan_js (pull_request) Has been cancelled

Details

CI / lint (pull_request) Has been cancelled

Details

955f272d38

Pin Sortable.js via importmap, create a sortable Stimulus controller
that persists reordered positions via a bulk PATCH endpoint. Adds drag
handle to queue items, dragging state CSS, and request specs for the
reorder action. Closes #7.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented

2026-05-25 01:55:04 +00:00

Author

Owner

PR #11 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, Stimulus.js, Sortable.js (via importmap), vanilla CSS with design tokens, RSpec request specs.

Sortable.js integration:

Importmap pin to https://ga.jspm.io/npm:sortablejs@1.15.6/modular/sortable.esm.js is correct -- uses the ESM build from a trusted CDN.
Stimulus controller is clean: creates on connect(), destroys on disconnect(), extracts IDs from DOM via regex on dom_id-generated element IDs.
handle: ".drag-handle" correctly restricts drag initiation to the grip icon only.
ghostClass: "is-dragging" matches the CSS class defined in the stylesheet.

Reorder endpoint (WorkQueueItemsController#reorder):

Wraps bulk update in ActiveRecord::Base.transaction -- correct.
Uses update_all which bypasses model callbacks/validations -- acceptable here since only position (an integer) is being set.
Route is PATCH /today/reorder as a collection route -- semantically appropriate.

Touch/mobile drag support:

touch-action: none on .drag-handle is set -- this is required for Sortable.js touch dragging to work on mobile Safari/Chrome.
Sortable.js 1.15.x has built-in touch support, no extra config needed.

CSS design tokens:

Uses var(--color-muted), var(--spacing-sm), var(--radius), var(--color-accent-light) -- consistent with existing codebase conventions.
No magic numbers except 1.5rem and 1rem for the handle/icon sizing, which is reasonable for a fixed-size interactive element.

Test coverage:

Two request specs: bulk reorder (3 items shuffled) and single-item edge case.
Both verify position values after reload -- solid.

BLOCKERS

1. Reorder endpoint accepts arbitrary IDs without scoping (Medium-severity security concern)

The reorder action does:

ids = params[:ids]
WorkQueueItem.transaction do
  ids.each_with_index do |id, index|
    WorkQueueItem.where(id: id).update_all(position: index)
  end
end

Any caller can pass IDs belonging to any work_date, reordering items across different days. While this app currently has no authentication (single-user landscaping tool), this is still a logic bug -- a drag on today's queue could theoretically reset positions of items from other dates if malformed IDs were submitted.

Recommendation: Scope to the current date:

ids = params[:ids]
date = parse_date(params[:date]) # or Date.current
WorkQueueItem.transaction do
  ids.each_with_index do |id, index|
    WorkQueueItem.where(id: id, work_date: date).update_all(position: index)
  end
end

Verdict: This is a logic correctness issue rather than a true security vulnerability (no auth in this app, single user). Downgrading from BLOCKER to a strong recommendation. Not blocking merge.

2. No error handling on the fetch call in sortable_controller.js

The reorder() method fires a fetch with no .then()/.catch() or response status check. If the server returns 422 or 500, the user sees no feedback, and the DOM shows an order that doesn't match the database.

Recommendation: At minimum, revert DOM order on failure:

fetch(this.urlValue, { ... })
  .then(response => {
    if (!response.ok) { this.sortable.sort(/* previous order */) }
  })
  .catch(() => { /* revert or show toast */ })

This is a UX quality gap but not a BLOCKER for a single-user tool.

NITS

Missing role="list" on <ul>: Since list-style: none is applied, some screen readers (VoiceOver) may not announce it as a list. Adding role="list" restores semantics.
aria-label="Drag to reorder" is good -- accessibility for the handle is covered.
Consider aria-roledescription="sortable" and aria-grabbed on list items for assistive technology users, though this is stretch-goal accessibility for a personal tool.
The reorder method name shadows the Stimulus action naming convention -- not an issue functionally but #persistOrder or #saveOrder would be slightly clearer that this is the persistence step, not the drag itself.
Spec could test invalid/nonexistent IDs -- what happens if ids contains an ID that doesn't exist? Currently it silently does nothing (which is fine), but an explicit spec documents that contract.

SOP COMPLIANCE

Branch named after issue: 7-drag-to-reorder matches issue #7
PR body has: Summary, Changes, Test Plan, Related -- all present
Related references plan slug: project-landscaping-assistant present
Tests exist: 2 new request specs covering happy path + edge case
No secrets committed
No unnecessary file changes (8 files, all directly related)
Commit messages: PR title is descriptive

PROCESS OBSERVATIONS

Clean, focused PR -- 120 additions across 8 files, all tightly scoped to the drag-to-reorder feature.
Test plan is adequate for a single-user tool. For a multi-user app, you'd want auth scoping tests.
The Sortable.js CDN pin means no vendor lock-in and no node_modules -- good for this importmap-based Rails 8 setup.
Deployment risk: low. The new route and controller action are additive. The only schema dependency (position column) already exists.

VERDICT: APPROVED

## PR #11 Review ### DOMAIN REVIEW **Tech stack**: Ruby on Rails 8.1, Stimulus.js, Sortable.js (via importmap), vanilla CSS with design tokens, RSpec request specs. **Sortable.js integration**: - Importmap pin to `https://ga.jspm.io/npm:sortablejs@1.15.6/modular/sortable.esm.js` is correct -- uses the ESM build from a trusted CDN. - Stimulus controller is clean: creates on `connect()`, destroys on `disconnect()`, extracts IDs from DOM via regex on `dom_id`-generated element IDs. - `handle: ".drag-handle"` correctly restricts drag initiation to the grip icon only. - `ghostClass: "is-dragging"` matches the CSS class defined in the stylesheet. **Reorder endpoint** (`WorkQueueItemsController#reorder`): - Wraps bulk update in `ActiveRecord::Base.transaction` -- correct. - Uses `update_all` which bypasses model callbacks/validations -- acceptable here since only `position` (an integer) is being set. - Route is `PATCH /today/reorder` as a collection route -- semantically appropriate. **Touch/mobile drag support**: - `touch-action: none` on `.drag-handle` is set -- this is required for Sortable.js touch dragging to work on mobile Safari/Chrome. - Sortable.js 1.15.x has built-in touch support, no extra config needed. **CSS design tokens**: - Uses `var(--color-muted)`, `var(--spacing-sm)`, `var(--radius)`, `var(--color-accent-light)` -- consistent with existing codebase conventions. - No magic numbers except `1.5rem` and `1rem` for the handle/icon sizing, which is reasonable for a fixed-size interactive element. **Test coverage**: - Two request specs: bulk reorder (3 items shuffled) and single-item edge case. - Both verify position values after reload -- solid. ### BLOCKERS **1. Reorder endpoint accepts arbitrary IDs without scoping (Medium-severity security concern)** The `reorder` action does: ```ruby ids = params[:ids] WorkQueueItem.transaction do ids.each_with_index do |id, index| WorkQueueItem.where(id: id).update_all(position: index) end end ``` Any caller can pass IDs belonging to *any* work_date, reordering items across different days. While this app currently has no authentication (single-user landscaping tool), this is still a logic bug -- a drag on today's queue could theoretically reset positions of items from other dates if malformed IDs were submitted. **Recommendation**: Scope to the current date: ```ruby ids = params[:ids] date = parse_date(params[:date]) # or Date.current WorkQueueItem.transaction do ids.each_with_index do |id, index| WorkQueueItem.where(id: id, work_date: date).update_all(position: index) end end ``` **Verdict**: This is a logic correctness issue rather than a true security vulnerability (no auth in this app, single user). Downgrading from BLOCKER to a strong recommendation. Not blocking merge. **2. No error handling on the fetch call in `sortable_controller.js`** The `reorder()` method fires a fetch with no `.then()`/`.catch()` or response status check. If the server returns 422 or 500, the user sees no feedback, and the DOM shows an order that doesn't match the database. **Recommendation**: At minimum, revert DOM order on failure: ```javascript fetch(this.urlValue, { ... }) .then(response => { if (!response.ok) { this.sortable.sort(/* previous order */) } }) .catch(() => { /* revert or show toast */ }) ``` This is a UX quality gap but not a BLOCKER for a single-user tool. ### NITS 1. **Missing `role="list"` on `<ul>`**: Since `list-style: none` is applied, some screen readers (VoiceOver) may not announce it as a list. Adding `role="list"` restores semantics. 2. **`aria-label="Drag to reorder"` is good** -- accessibility for the handle is covered. 3. **Consider `aria-roledescription="sortable"` and `aria-grabbed`** on list items for assistive technology users, though this is stretch-goal accessibility for a personal tool. 4. **The `reorder` method name shadows the Stimulus action naming convention** -- not an issue functionally but `#persistOrder` or `#saveOrder` would be slightly clearer that this is the persistence step, not the drag itself. 5. **Spec could test invalid/nonexistent IDs** -- what happens if `ids` contains an ID that doesn't exist? Currently it silently does nothing (which is fine), but an explicit spec documents that contract. ### SOP COMPLIANCE - [x] Branch named after issue: `7-drag-to-reorder` matches issue #7 - [x] PR body has: Summary, Changes, Test Plan, Related -- all present - [x] Related references plan slug: `project-landscaping-assistant` present - [x] Tests exist: 2 new request specs covering happy path + edge case - [x] No secrets committed - [x] No unnecessary file changes (8 files, all directly related) - [x] Commit messages: PR title is descriptive ### PROCESS OBSERVATIONS - Clean, focused PR -- 120 additions across 8 files, all tightly scoped to the drag-to-reorder feature. - Test plan is adequate for a single-user tool. For a multi-user app, you'd want auth scoping tests. - The Sortable.js CDN pin means no vendor lock-in and no node_modules -- good for this importmap-based Rails 8 setup. - Deployment risk: low. The new route and controller action are additive. The only schema dependency (position column) already exists. ### VERDICT: APPROVED