pal-e-docs

Review: Create clicks table and tracking

review-1400-2026-06-09 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- references spike #16, prerequisite for #40
  • [x] Repo -- ldraney/palinks
  • [x] User Story -- well-formed As/I want/So that
  • [x] Context -- thorough, includes schema from spike doc and key design decisions
  • [x] File Targets -- 8 targets with modify/create and NOT touch sections
  • [x] Feature Flag -- "none" with rationale (deferred to #34)
  • [x] Acceptance Criteria -- 6 criteria
  • [x] Test Expectations -- 5 expectations with run command
  • [x] Constraints -- 4 constraints, well-specified
  • [x] Checklist -- present
  • [x] Related -- present, references #40 and #41

Traceability

  • [x] story:auth-roles label -- Keycloak auth, roles, visibility, feature flags
  • [x] story note verified -- found in project-palinks user-stories section
  • [x] arch:palinks label -- palinks architecture component
  • [ ] arch note MISSING -- [SCOPE] Create architecture note arch-palinks for the palinks component. search_notes("arch-palinks") returned no results.
  • [x] Forgejo issue -- https://forgejo.tail5b443a.ts.net/ldraney/palinks/issues/36, open

File Targets

  • [x] db/migrate/*_create_clicks.rb -- verified: NEW file, no existing clicks migration. 4 existing migrations in db/migrate/.
  • [x] app/models/click.rb -- verified: NEW file. Only link.rb and application_record.rb exist in app/models/.
  • [x] app/models/link.rb -- verified: EXISTS. Currently has validations, scopes (ordered, favorites, tagged). No existing click associations.
  • [x] app/controllers/clicks_controller.rb -- verified: NEW file. Only links_controller.rb and application_controller.rb exist.
  • [x] config/routes.rb -- verified: EXISTS. Currently has resources :links with sort and toggle_favorite. Click route will nest under links.
  • [x] app/views/links/_link.html.erb -- verified: EXISTS. Line 13: link_to link.title, link.url links directly to URL. Correctly identified for modification.
  • [x] test/models/click_test.rb -- verified: NEW file. Note: test/models/ directory does not exist yet and must be created.
  • [x] test/controllers/clicks_controller_test.rb -- verified: NEW file. test/controllers/ directory exists.

File Target Issues

  • [ ] app/views/links/show.html.erb -- ISSUE: [BODY] Line 4 also links directly to link.url via link_to @link.title, @link.url. This view should either also route through the click tracking endpoint or be explicitly listed in "Files the agent should NOT touch" with rationale. Currently unaddressed -- an agent may miss it or make an inconsistent choice.

Repo Placement

OK. Issue is filed on ldraney/palinks, all file targets are within that repo. Single-repo scope.

Dependencies

  • Upstream (not blocking): #32 (users table) -- ticket correctly handles this by making user_id a plain bigint with no FK constraint. Good.
  • Upstream (not blocking): #34 (feature_flags table) -- ticket correctly defers flag gating. Feature Flag section documents this with rationale.
  • Downstream (blocked by this): #40 (canvas view with activity-based card sizing, board item #1406) -- needs click_count data from this ticket. Documented in Lineage and Related sections.
  • Downstream (transitively): #41 (link groups, board item #1407) -- depends on #40 which depends on this.
  • No items currently in_progress that block this ticket.
  • Dependencies are well-documented in the issue body.

Acceptance Criteria

All 6 criteria are agent-verifiable:
  • [x] "Clicking a link card records a click row" -- testable via integration test POST
  • [x] "Anonymous clicks record session_id" -- testable via controller test checking session assignment
  • [x] "After recording, user is redirected" -- testable via assert_redirected_to
  • [x] "Link#click_count returns total clicks" -- testable via unit test
  • [x] "Click recording does not break existing link card UI" -- testable via existing test suite passing
  • [x] "user_id column exists but nullable, no FK" -- testable via schema inspection / migration review
Test expectations align well with acceptance criteria. Run command bin/rails test is correct for this project.

Blast Radius

  • show.html.erb: Also links directly to link.url (line 4). If _link.html.erb is updated to route through click tracking, show.html.erb should be addressed too -- either tracked or explicitly excluded.
  • No sibling services affected: palinks is a standalone Rails app. No other services consume the links table.
  • Existing sort test: test/controllers/links_controller_sort_test.rb exists. The new click route should not break existing link routes, but the agent should verify existing tests still pass.
  • Turbo/JS interaction: The constraint says "use redirect, not AJAX" which is correct given the existing Turbo setup. The POST-and-redirect pattern avoids Turbo stream complications.

Decomposition Assessment

8 file targets in 1 repo. 6 acceptance criteria (slightly over the 5 threshold). However, this is a standard Rails resource scaffold pattern (migration + model + controller + route + view update + tests) -- cohesive and well-understood. An experienced agent can complete this in a single pass within 5 minutes. No decomposition needed.

Recommendations

  • [BODY] Add app/views/links/show.html.erb to File Targets (either as a file to modify for click tracking consistency, or explicitly in the "should NOT touch" list with rationale for why show page clicks are not tracked).
  • [SCOPE] Create architecture note arch-palinks for the palinks component in pal-e-docs. This is a cross-cutting gap affecting all palinks board items, not just this ticket.