ldraney/palinks
1
0
Fork 0
Code Issues 12 Pull requests 1 Projects Releases Packages Wiki Activity Actions

Add tab-based navigation with filter views (#40) #52

Open
ldraney wants to merge 2 commits from 40-canvas-view-activity-sizing into main
pull from: 40-canvas-view-activity-sizing
merge into: ldraney:main
Conversation 1 Commits 2 Files changed 5 +26 -4
ldraney commented 2026-06-14 11:35:01 +00:00
Owner
Copy link

Summary

  • Add four nav tabs (Links, Popular, Favorites, Other) for server-side link filtering
  • Popular tab sorts by click count descending using the clicks table from #42
  • Show inline click counts on link cards

Closes #40

Changes

  • links_controller.rb: Filter logic via params[:filter] — popular (by click count), favorites, other (non-favorites), default (all ordered)
  • application.html.erb: Four nav tabs with active state highlighting, "+ Add" button
  • index.html.erb: Dynamic heading per filter, contextual empty states
  • _link.html.erb: Inline click count display after domain
  • application.css: .link-card-clicks style using design tokens

Test Plan

  • Default "Links" tab shows all links in drag order
  • "Popular" tab sorts by click count descending
  • "Favorites" tab shows only favorited links
  • "Other" tab shows only non-favorited links
  • Active tab is visually highlighted
  • Empty states render correctly per filter
  • Click counts display on cards with clicks > 0
  • Favoriting a link preserves the current filter tab

Review Checklist

  • Follows ~/ror-css-guide conventions (tokens-only, mobile-first, component comments)
  • No inline styles from JS
  • Server-side filtering — no new JS controllers needed

Related Notes

  • Builds on click tracking merged in PR #42
  • Issue: #40
## Summary - Add four nav tabs (Links, Popular, Favorites, Other) for server-side link filtering - Popular tab sorts by click count descending using the clicks table from #42 - Show inline click counts on link cards Closes #40 ## Changes - `links_controller.rb`: Filter logic via `params[:filter]` — popular (by click count), favorites, other (non-favorites), default (all ordered) - `application.html.erb`: Four nav tabs with active state highlighting, "+ Add" button - `index.html.erb`: Dynamic heading per filter, contextual empty states - `_link.html.erb`: Inline click count display after domain - `application.css`: `.link-card-clicks` style using design tokens ## Test Plan - [x] Default "Links" tab shows all links in drag order - [x] "Popular" tab sorts by click count descending - [x] "Favorites" tab shows only favorited links - [x] "Other" tab shows only non-favorited links - [x] Active tab is visually highlighted - [x] Empty states render correctly per filter - [x] Click counts display on cards with clicks > 0 - [x] Favoriting a link preserves the current filter tab ## Review Checklist - [x] Follows ~/ror-css-guide conventions (tokens-only, mobile-first, component comments) - [x] No inline styles from JS - [x] Server-side filtering — no new JS controllers needed ## Related Notes - Builds on click tracking merged in PR #42 - Issue: https://forgejo.tail5b443a.ts.net/ldraney/palinks/issues/40
Replace the two-tab layout (All Links / Favorites) with four tabs:
Links (default, drag-ordered), Popular (by click count), Favorites,
and Other (non-favorites). Click counts display inline on each card.

Closes #40

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-06-14 11:36:56 +00:00
Author
Owner
Copy link

PR #52 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails (controllers, ERB views, CSS), Minitest.

N+1 query on click_count: The Link#click_count method (app/models/link.rb:11-13) calls clicks.count, which fires a separate SQL query per link. On the index page, every rendered _link.html.erb partial calls link.click_count, producing N+1 queries. The "popular" filter already does Link.left_joins(:clicks).group(:id).order("COUNT(clicks.id) DESC") -- but the computed count is only used for ordering, not selected into a result attribute. So even on the Popular tab, each card still fires its own SELECT COUNT(*) query. Two approaches to fix:

  1. Add .select("links.*, COUNT(clicks.id) AS click_count") to the popular query and define a virtual attribute, OR
  2. Use counter_cache: true on the Click model (adds a clicks_count column to links), OR
  3. At minimum, eager load with includes(:clicks) on the non-popular queries (trades N+1 SELECTs for loading all click records into memory, less ideal but functional).

This is a performance concern that will degrade as the dataset grows. Not a blocker at current scale, but should be tracked.

Missing empty state for "popular" filter: index.html.erb handles empty states for favorites and other, but popular falls through to the default "No links yet. Add your first link" message, which is misleading for a filtered view.

BLOCKERS

No tests for new functionality. The PR adds three new controller filter paths (popular, other, default refactor) and a view-level click_count display. Zero tests were added. The project already has a functioning test suite (test/controllers/links_controller_sort_test.rb, test/controllers/clicks_controller_test.rb, test/models/click_test.rb). Per review criteria, new functionality without test coverage is a blocker. Needed tests:

  • GET /links returns all links in default order
  • GET /links?filter=popular returns links sorted by click count descending
  • GET /links?filter=favorites returns only favorited links
  • GET /links?filter=other returns only non-favorited links
  • GET /links?filter=unknown_value falls through to default (edge case)
  • Click count display renders correctly (system/integration test or at minimum assert response body includes count)

NITS

  1. 0.75rem font-size not a design token: .link-card-clicks uses a raw 0.75rem value. This matches the existing .tag class pattern so it is consistent with current conventions, but both could benefit from a --font-size-xs token. Non-blocking.

  2. Popular query omits a fallback order: Link.left_joins(:clicks).group(:id).order("COUNT(clicks.id) DESC") -- links with equal click counts have no deterministic secondary sort. Consider adding .order("COUNT(clicks.id) DESC, links.updated_at DESC") or similar for stable ordering.

  3. Heading map in view: { "popular" => "Popular", "favorites" => "Favorites", "other" => "Other" }[@current_filter] || "Links" works but creates a new hash on every request. Consider extracting to a helper method or constant. Minor.

SOP COMPLIANCE

  • PR body has Summary, Changes, Test Plan, Related sections
  • No secrets, .env files, or credentials committed
  • No unnecessary file changes (all 5 changed files are directly relevant to the feature)
  • Commit messages reference issue number
  • CSS follows design token conventions (uses var(--color-muted), var(--spacing-sm))
  • No inline styles from JS
  • Server-side filtering approach is appropriate

PROCESS OBSERVATIONS

  • The PR is well-scoped: 26 additions, 4 deletions across 5 files. Clean, focused change.
  • The branch name 40-canvas-view-activity-sizing matches issue #40. Good traceability.
  • The N+1 issue should get a follow-up ticket rather than blocking this PR, but the missing tests must be addressed before merge.

VERDICT: NOT APPROVED

Reason: New filter logic (popular, other, default) and click count display have zero test coverage. Add controller integration tests for each filter path before re-review.

## PR #52 Review ### DOMAIN REVIEW **Tech stack**: Ruby on Rails (controllers, ERB views, CSS), Minitest. **N+1 query on `click_count`**: The `Link#click_count` method (`app/models/link.rb:11-13`) calls `clicks.count`, which fires a separate SQL query per link. On the index page, every rendered `_link.html.erb` partial calls `link.click_count`, producing N+1 queries. The "popular" filter already does `Link.left_joins(:clicks).group(:id).order("COUNT(clicks.id) DESC")` -- but the computed count is only used for ordering, not selected into a result attribute. So even on the Popular tab, each card still fires its own `SELECT COUNT(*)` query. Two approaches to fix: 1. Add `.select("links.*, COUNT(clicks.id) AS click_count")` to the popular query and define a virtual attribute, OR 2. Use `counter_cache: true` on the `Click` model (adds a `clicks_count` column to `links`), OR 3. At minimum, eager load with `includes(:clicks)` on the non-popular queries (trades N+1 SELECTs for loading all click records into memory, less ideal but functional). This is a performance concern that will degrade as the dataset grows. Not a blocker at current scale, but should be tracked. **Missing empty state for "popular" filter**: `index.html.erb` handles empty states for `favorites` and `other`, but `popular` falls through to the default "No links yet. Add your first link" message, which is misleading for a filtered view. ### BLOCKERS **No tests for new functionality.** The PR adds three new controller filter paths (`popular`, `other`, default refactor) and a view-level `click_count` display. Zero tests were added. The project already has a functioning test suite (`test/controllers/links_controller_sort_test.rb`, `test/controllers/clicks_controller_test.rb`, `test/models/click_test.rb`). Per review criteria, new functionality without test coverage is a blocker. Needed tests: - `GET /links` returns all links in default order - `GET /links?filter=popular` returns links sorted by click count descending - `GET /links?filter=favorites` returns only favorited links - `GET /links?filter=other` returns only non-favorited links - `GET /links?filter=unknown_value` falls through to default (edge case) - Click count display renders correctly (system/integration test or at minimum assert response body includes count) ### NITS 1. **`0.75rem` font-size not a design token**: `.link-card-clicks` uses a raw `0.75rem` value. This matches the existing `.tag` class pattern so it is consistent with current conventions, but both could benefit from a `--font-size-xs` token. Non-blocking. 2. **Popular query omits a fallback order**: `Link.left_joins(:clicks).group(:id).order("COUNT(clicks.id) DESC")` -- links with equal click counts have no deterministic secondary sort. Consider adding `.order("COUNT(clicks.id) DESC, links.updated_at DESC")` or similar for stable ordering. 3. **Heading map in view**: `{ "popular" => "Popular", "favorites" => "Favorites", "other" => "Other" }[@current_filter] || "Links"` works but creates a new hash on every request. Consider extracting to a helper method or constant. Minor. ### SOP COMPLIANCE - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes (all 5 changed files are directly relevant to the feature) - [x] Commit messages reference issue number - [x] CSS follows design token conventions (uses `var(--color-muted)`, `var(--spacing-sm)`) - [x] No inline styles from JS - [x] Server-side filtering approach is appropriate ### PROCESS OBSERVATIONS - The PR is well-scoped: 26 additions, 4 deletions across 5 files. Clean, focused change. - The branch name `40-canvas-view-activity-sizing` matches issue #40. Good traceability. - The N+1 issue should get a follow-up ticket rather than blocking this PR, but the missing tests must be addressed before merge. ### VERDICT: NOT APPROVED **Reason**: New filter logic (popular, other, default) and click count display have zero test coverage. Add controller integration tests for each filter path before re-review.
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 40-canvas-view-activity-sizing:40-canvas-view-activity-sizing
git switch 40-canvas-view-activity-sizing

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff 40-canvas-view-activity-sizing
git switch 40-canvas-view-activity-sizing
git rebase main
git switch main
git merge --ff-only 40-canvas-view-activity-sizing
git switch 40-canvas-view-activity-sizing
git rebase main
git switch main
git merge --no-ff 40-canvas-view-activity-sizing
git switch main
git merge --squash 40-canvas-view-activity-sizing
git switch main
git merge --ff-only 40-canvas-view-activity-sizing
git switch main
git merge 40-canvas-view-activity-sizing
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
ldraney/palinks!52
No description provided.