Add drag-and-drop reordering for link cards

ldraney commented

2026-05-25 11:28:09 +00:00

Author

Owner

PR #10 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, Stimulus.js (Hotwire), importmap-rails, PostgreSQL, Propshaft. This is a Rails + Stimulus review.

Migration (db/migrate/20260524194416_add_position_to_links.rb): Clean. integer, default: 0, null: false is correct. All existing rows get position 0, which is fine since ordered scope breaks ties with updated_at: :desc. One consideration: for a large table this would benefit from a database index on position, but given this is a personal links app with low cardinality, this is a nit, not a blocker.

Model (app/models/link.rb): Scope change from order(updated_at: :desc) to order(position: :asc, updated_at: :desc) is correct. Composite ordering with tie-breaking is good.

Routes (config/routes.rb): patch :sort, on: :collection produces PATCH /links/sort -- idiomatic Rails for a collection-level action. Correct.

Controller (app/controllers/links_controller.rb):

The sort action is the most important piece to scrutinize:

def sort
  params[:ids].each_with_index do |id, index|
    Link.where(id: id).update_all(position: index)
  end
  head :no_content
end

Findings:

Input validation -- no type checking on params[:ids] (see BLOCKERS). The action iterates params[:ids] without validating that it is an array or that each element is a valid integer. A malformed request body (e.g., ids as a string, or containing non-integer values) will silently pass through or raise an unhandled error. Link.where(id: id) with ActiveRecord does parameterize the query (preventing SQL injection), but the lack of any validation is still a concern for robustness.
N+1 queries: Each ID issues a separate UPDATE statement. For N links, this is N queries in a loop. Consider a single CASE update or update_all with a values list. Not a blocker for a small personal app, but worth noting.
No authentication/authorization on sort: The app has no authentication layer at all (ApplicationController has no auth concern), so this is consistent with the rest of the app. Not a regression. However, the sort endpoint accepts arbitrary IDs -- a caller can submit IDs for links they should not reorder. Since there is no auth anywhere in the app, this is noted but not a blocker specific to this PR.
No transaction wrapping: If the server crashes mid-loop, positions will be partially updated. Wrapping in ActiveRecord::Base.transaction would be safer.

Stimulus Controller (app/javascript/controllers/sortable_controller.js):

Well-structured. Dual desktop (HTML5 DnD) and mobile (touch) support is solid. Observations:

disconnect() does not remove touch event listeners from handles. The connect() method adds touchstart, touchmove, and touchend listeners on this.handleTargets, but disconnect() only removes the six drag listeners from this.itemTargets. This is a memory leak if Turbo navigates away and reconnects. Should add corresponding removeEventListener calls for the three touch events.
persist() does not handle fetch failures. If the PATCH fails (network error, 500, etc.), the DOM reorder sticks but the server state diverges. The user sees the new order but on reload it reverts. Consider a .catch() that reverts the DOM or shows a flash.
Hardcoded URL: fetch("/links/sort", ...) has a hardcoded path. If the app is ever mounted at a subpath, this breaks. A data-sortable-url-value Stimulus value sourced from sort_links_path in the template would be more robust. Low risk for this app.
CSRF token lookup (meta[name='csrf-token']): This is correct -- the layout includes <%= csrf_meta_tags %> which renders that meta tag. Good.

Importmap setup (config/importmap.rb, app/javascript/): Standard Rails 8 importmap + Stimulus bootstrap. pin_all_from for controllers is correct. javascript_importmap_tags added to layout. stale_when_importmap_changes already present in ApplicationController. All good.

View changes (_link.html.erb, index.html.erb): Clean restructuring. The drag handle ☰ (hamburger/trigram) is a reasonable icon choice. data-sortable-target="item" and data-sortable-target="handle" correctly wire up the Stimulus targets.

CSS (application.css): Uses CSS custom properties (theme tokens) consistently. .is-dragging, .is-drag-over states are well-defined. touch-action: none on the handle prevents browser scroll interference during touch drag. user-select: none prevents text selection. Layout switch to flex with gap is clean.

docker-compose.yml: Port remap 9998:9999 -- operational preference, not a code quality concern. Fine.

development.rb: Adding dev-tunnel-palinks.tail5b443a.ts.net to config.hosts -- this is a Tailscale dev tunnel hostname. Development-only, no security concern.

BLOCKERS

Missing touch event listener cleanup in disconnect() (sortable_controller.js line 24-33). The connect() method attaches three touch event listeners (touchstart, touchmove, touchend) to each handle target, but disconnect() never removes them. This is a genuine bug -- when Turbo navigates away and the controller disconnects, those listeners remain attached. On reconnection, duplicate listeners accumulate. This will cause erratic behavior (e.g., the same touch triggering multiple drag operations) and constitutes a memory/event leak. Must be fixed.
No input validation on sort action (links_controller.rb line 36-41). params[:ids] is consumed with no validation that it is an array of integers, no empty-array guard, and no size bounds. While ActiveRecord parameterizes the WHERE id = ? clause (preventing SQL injection), the action should at minimum: (a) verify params[:ids] is present and is an array, (b) cast or filter to integers, (c) optionally verify all IDs exist. A malformed request will produce a 500 (calling each_with_index on nil if :ids is missing), which is poor API behavior for a public-facing endpoint.

NITS

N+1 updates in sort: Each ID fires a separate UPDATE. A single SQL CASE statement or update_all with values could batch this. Low impact at current scale.
No transaction around the sort loop: Wrapping the position updates in ActiveRecord::Base.transaction { ... } would make the operation atomic.
Hardcoded /links/sort URL in persist(): Consider using a Stimulus value attribute sourced from a Rails path helper for maintainability.
Missing fetch error handling in persist(): The Promise returned by fetch() is fire-and-forget. A .catch() to log or show user feedback on failure would improve UX.
No index on position column: For a personal app with a handful of links this is fine, but if the dataset grows, ORDER BY position benefits from an index.
Accessibility: The drag handle <span> with ☰ has no ARIA label or role attribute. Screen readers will either skip it or read the Unicode character name. Adding role="button" and aria-label="Reorder" would improve accessibility.

SOP COMPLIANCE

Branch naming: Branch is feature/sortable-link-cards. Expected convention is {issue-number}-{kebab-case-purpose} (e.g., 9-drag-drop-reorder). Does not follow convention.
PR body follows template: Has Summary, Changes, Test Plan, Related sections. Well-structured.
Related references plan slug: PR references issue #9 but no plan slug is referenced (confirmed: no plan slug exists for this work).
No secrets committed: Confirmed. Docker-compose has rails/rails dev credentials which are standard for local dev.
No unnecessary file changes: All 16 changed files are relevant to the feature. The docker-compose port change and dev host addition are minor operational tweaks that could have been separate, but are not scope creep per se.
Commit messages: (Not individually visible in diff, but PR title and body are descriptive.)

PROCESS OBSERVATIONS

Test coverage: There is no test/ or spec/ directory in the repository at all. The entire app has zero automated tests. While this PR's Test Plan describes manual verification steps, the absence of any automated test infrastructure means there is no safety net for regressions. This is a pre-existing condition, not introduced by this PR, but it compounds risk as features are added.
Change failure risk: The sort controller action is the highest-risk addition. Without input validation, a malformed client request can produce a 500 error. Without a transaction, a partial failure leaves positions inconsistent.
Deployment: Migration is additive (new column with default), so it is safe to run against a live database with no downtime risk.

VERDICT: NOT APPROVED

Two blockers must be addressed before merge:

Fix the touch event listener leak in disconnect() -- add removeEventListener for touchstart, touchmove, touchend on handle targets.
Add input validation to the sort controller action -- verify params[:ids] is present, is an array, and contains valid values before iterating.

## PR #10 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails 8.1, Stimulus.js (Hotwire), importmap-rails, PostgreSQL, Propshaft. This is a Rails + Stimulus review. **Migration** (`db/migrate/20260524194416_add_position_to_links.rb`): Clean. `integer, default: 0, null: false` is correct. All existing rows get position 0, which is fine since `ordered` scope breaks ties with `updated_at: :desc`. One consideration: for a large table this would benefit from a database index on `position`, but given this is a personal links app with low cardinality, this is a nit, not a blocker. **Model** (`app/models/link.rb`): Scope change from `order(updated_at: :desc)` to `order(position: :asc, updated_at: :desc)` is correct. Composite ordering with tie-breaking is good. **Routes** (`config/routes.rb`): `patch :sort, on: :collection` produces `PATCH /links/sort` -- idiomatic Rails for a collection-level action. Correct. **Controller** (`app/controllers/links_controller.rb`): The `sort` action is the most important piece to scrutinize: ```ruby def sort params[:ids].each_with_index do |id, index| Link.where(id: id).update_all(position: index) end head :no_content end ``` Findings: 1. **Input validation -- no type checking on `params[:ids]`** (see BLOCKERS). The action iterates `params[:ids]` without validating that it is an array or that each element is a valid integer. A malformed request body (e.g., `ids` as a string, or containing non-integer values) will silently pass through or raise an unhandled error. `Link.where(id: id)` with ActiveRecord does parameterize the query (preventing SQL injection), but the lack of any validation is still a concern for robustness. 2. **N+1 queries**: Each ID issues a separate `UPDATE` statement. For N links, this is N queries in a loop. Consider a single `CASE` update or `update_all` with a values list. Not a blocker for a small personal app, but worth noting. 3. **No authentication/authorization on sort**: The app has no authentication layer at all (`ApplicationController` has no auth concern), so this is consistent with the rest of the app. Not a regression. However, the sort endpoint accepts arbitrary IDs -- a caller can submit IDs for links they should not reorder. Since there is no auth anywhere in the app, this is noted but not a blocker specific to this PR. 4. **No transaction wrapping**: If the server crashes mid-loop, positions will be partially updated. Wrapping in `ActiveRecord::Base.transaction` would be safer. **Stimulus Controller** (`app/javascript/controllers/sortable_controller.js`): Well-structured. Dual desktop (HTML5 DnD) and mobile (touch) support is solid. Observations: 1. **`disconnect()` does not remove touch event listeners from handles.** The `connect()` method adds `touchstart`, `touchmove`, and `touchend` listeners on `this.handleTargets`, but `disconnect()` only removes the six drag listeners from `this.itemTargets`. This is a memory leak if Turbo navigates away and reconnects. Should add corresponding `removeEventListener` calls for the three touch events. 2. **`persist()` does not handle fetch failures.** If the PATCH fails (network error, 500, etc.), the DOM reorder sticks but the server state diverges. The user sees the new order but on reload it reverts. Consider a `.catch()` that reverts the DOM or shows a flash. 3. **Hardcoded URL**: `fetch("/links/sort", ...)` has a hardcoded path. If the app is ever mounted at a subpath, this breaks. A `data-sortable-url-value` Stimulus value sourced from `sort_links_path` in the template would be more robust. Low risk for this app. 4. **CSRF token lookup** (`meta[name='csrf-token']`): This is correct -- the layout includes `<%= csrf_meta_tags %>` which renders that meta tag. Good. **Importmap setup** (`config/importmap.rb`, `app/javascript/`): Standard Rails 8 importmap + Stimulus bootstrap. `pin_all_from` for controllers is correct. `javascript_importmap_tags` added to layout. `stale_when_importmap_changes` already present in `ApplicationController`. All good. **View changes** (`_link.html.erb`, `index.html.erb`): Clean restructuring. The drag handle `☰` (hamburger/trigram) is a reasonable icon choice. `data-sortable-target="item"` and `data-sortable-target="handle"` correctly wire up the Stimulus targets. **CSS** (`application.css`): Uses CSS custom properties (theme tokens) consistently. `.is-dragging`, `.is-drag-over` states are well-defined. `touch-action: none` on the handle prevents browser scroll interference during touch drag. `user-select: none` prevents text selection. Layout switch to flex with `gap` is clean. **docker-compose.yml**: Port remap `9998:9999` -- operational preference, not a code quality concern. Fine. **development.rb**: Adding `dev-tunnel-palinks.tail5b443a.ts.net` to `config.hosts` -- this is a Tailscale dev tunnel hostname. Development-only, no security concern. ### BLOCKERS 1. **Missing touch event listener cleanup in `disconnect()`** (`sortable_controller.js` line 24-33). The `connect()` method attaches three touch event listeners (`touchstart`, `touchmove`, `touchend`) to each handle target, but `disconnect()` never removes them. This is a genuine bug -- when Turbo navigates away and the controller disconnects, those listeners remain attached. On reconnection, duplicate listeners accumulate. This will cause erratic behavior (e.g., the same touch triggering multiple drag operations) and constitutes a memory/event leak. Must be fixed. 2. **No input validation on `sort` action** (`links_controller.rb` line 36-41). `params[:ids]` is consumed with no validation that it is an array of integers, no empty-array guard, and no size bounds. While ActiveRecord parameterizes the `WHERE id = ?` clause (preventing SQL injection), the action should at minimum: (a) verify `params[:ids]` is present and is an array, (b) cast or filter to integers, (c) optionally verify all IDs exist. A malformed request will produce a 500 (calling `each_with_index` on `nil` if `:ids` is missing), which is poor API behavior for a public-facing endpoint. ### NITS 1. **N+1 updates in `sort`**: Each ID fires a separate `UPDATE`. A single SQL `CASE` statement or `update_all` with values could batch this. Low impact at current scale. 2. **No transaction around the sort loop**: Wrapping the position updates in `ActiveRecord::Base.transaction { ... }` would make the operation atomic. 3. **Hardcoded `/links/sort` URL** in `persist()`: Consider using a Stimulus value attribute sourced from a Rails path helper for maintainability. 4. **Missing `fetch` error handling** in `persist()`: The Promise returned by `fetch()` is fire-and-forget. A `.catch()` to log or show user feedback on failure would improve UX. 5. **No index on `position` column**: For a personal app with a handful of links this is fine, but if the dataset grows, `ORDER BY position` benefits from an index. 6. **Accessibility**: The drag handle `<span>` with `☰` has no ARIA label or `role` attribute. Screen readers will either skip it or read the Unicode character name. Adding `role="button"` and `aria-label="Reorder"` would improve accessibility. ### SOP COMPLIANCE - [ ] **Branch naming**: Branch is `feature/sortable-link-cards`. Expected convention is `{issue-number}-{kebab-case-purpose}` (e.g., `9-drag-drop-reorder`). Does not follow convention. - [x] **PR body follows template**: Has Summary, Changes, Test Plan, Related sections. Well-structured. - [ ] **Related references plan slug**: PR references issue #9 but no plan slug is referenced (confirmed: no plan slug exists for this work). - [x] **No secrets committed**: Confirmed. Docker-compose has `rails`/`rails` dev credentials which are standard for local dev. - [x] **No unnecessary file changes**: All 16 changed files are relevant to the feature. The docker-compose port change and dev host addition are minor operational tweaks that could have been separate, but are not scope creep per se. - [x] **Commit messages**: (Not individually visible in diff, but PR title and body are descriptive.) ### PROCESS OBSERVATIONS - **Test coverage**: There is no `test/` or `spec/` directory in the repository at all. The entire app has zero automated tests. While this PR's Test Plan describes manual verification steps, the absence of any automated test infrastructure means there is no safety net for regressions. This is a pre-existing condition, not introduced by this PR, but it compounds risk as features are added. - **Change failure risk**: The `sort` controller action is the highest-risk addition. Without input validation, a malformed client request can produce a 500 error. Without a transaction, a partial failure leaves positions inconsistent. - **Deployment**: Migration is additive (new column with default), so it is safe to run against a live database with no downtime risk. ### VERDICT: NOT APPROVED Two blockers must be addressed before merge: 1. Fix the touch event listener leak in `disconnect()` -- add `removeEventListener` for `touchstart`, `touchmove`, `touchend` on handle targets. 2. Add input validation to the `sort` controller action -- verify `params[:ids]` is present, is an array, and contains valid values before iterating.

2026-05-25 11:26:07 +00:00

Summary

Add position column and Stimulus sortable controller for drag-and-drop card reordering
Set up importmap + Stimulus JS pipeline (was missing entirely)
Support desktop (HTML5 DnD) and mobile (touch via drag handle with auto-scroll)

Changes

db/migrate/*_add_position_to_links.rb: add position integer column (default 0, not null)
app/models/link.rb: ordered scope now sorts by position asc, updated_at desc
app/controllers/links_controller.rb: add sort action, accepts ordered IDs via PATCH
config/routes.rb: add patch :sort, on: :collection to links resource
config/importmap.rb: new -- pins for Turbo, Stimulus, controllers
app/javascript/: new -- application.js, Stimulus bootstrap, sortable_controller.js
app/views/links/_link.html.erb: add drag handle, wrap body in flex layout
app/views/links/index.html.erb: add data-controller="sortable" to grid
app/views/layouts/application.html.erb: add javascript_importmap_tags
app/assets/stylesheets/application.css: compact cards, handle styles, .is-dragging/.is-drag-over states
config/environments/development.rb: allow dev-tunnel-palinks host
docker-compose.yml: remap to port 9998 (avoids conflict with landscaping-assistant)

Test Plan

Desktop: drag card to new position, reload, verify order persisted
Mobile: touch-drag via handle, verify reorder without blocking scroll
Mobile: drag near viewport edge, verify auto-scroll
bin/rails db:migrate runs cleanly
No JS console errors on page load

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

ldraney/palinks #9 -- the Forgejo issue this PR implements
Closes #9

## Summary - Add `position` column and Stimulus sortable controller for drag-and-drop card reordering - Set up importmap + Stimulus JS pipeline (was missing entirely) - Support desktop (HTML5 DnD) and mobile (touch via drag handle with auto-scroll) ## Changes - `db/migrate/*_add_position_to_links.rb`: add position integer column (default 0, not null) - `app/models/link.rb`: ordered scope now sorts by position asc, updated_at desc - `app/controllers/links_controller.rb`: add `sort` action, accepts ordered IDs via PATCH - `config/routes.rb`: add `patch :sort, on: :collection` to links resource - `config/importmap.rb`: new -- pins for Turbo, Stimulus, controllers - `app/javascript/`: new -- application.js, Stimulus bootstrap, sortable_controller.js - `app/views/links/_link.html.erb`: add drag handle, wrap body in flex layout - `app/views/links/index.html.erb`: add `data-controller="sortable"` to grid - `app/views/layouts/application.html.erb`: add `javascript_importmap_tags` - `app/assets/stylesheets/application.css`: compact cards, handle styles, `.is-dragging`/`.is-drag-over` states - `config/environments/development.rb`: allow dev-tunnel-palinks host - `docker-compose.yml`: remap to port 9998 (avoids conflict with landscaping-assistant) ## Test Plan - [ ] Desktop: drag card to new position, reload, verify order persisted - [ ] Mobile: touch-drag via handle, verify reorder without blocking scroll - [ ] Mobile: drag near viewport edge, verify auto-scroll - [ ] `bin/rails db:migrate` runs cleanly - [ ] No JS console errors on page load ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - `ldraney/palinks #9` -- the Forgejo issue this PR implements - Closes #9

ldraney added 1 commit

Add drag-and-drop reordering for link cards 086ea3757d

Adds a position column to links and a Stimulus sortable controller
that supports both desktop (HTML5 DnD) and mobile (touch events via
drag handle). Cards are more compact with a visible grab handle.
Also sets up the importmap/Stimulus JS pipeline and adds a dev tunnel
overlay for the palinks service.

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

2026-05-25 11:28:47 +00:00

Fix review blockers: touch listener leak and sort input validation 0861522780

Clean up touch event listeners in disconnect() to prevent accumulation
on Turbo navigation. Validate and sanitize params[:ids] in the sort
action, wrap updates in a transaction, and return 400 on bad input.

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