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

Fix link cards navigating away from palinks #44

Merged
ldraney merged 2 commits from fix-link-click-redirect into main 2026-06-13 06:27:31 +00:00
Conversation 1 Commits 2 Files changed 2 +3 -2
ldraney commented 2026-06-13 06:23:32 +00:00
Owner
Copy link

Summary

  • Link card titles now navigate to the show page instead of the external URL
  • Show page displays a "Visit" button that opens the external URL in a new tab

Changes

  • app/views/links/_link.html.erb -- title links to link_path(link) instead of link.url
  • app/views/links/show.html.erb -- title is plain text, added "Visit" button with target="_blank"

Test Plan

  • Click link card title, verify show page loads
  • Click "Visit" button on show page, verify external URL opens in new tab
  • Verify card layout and styling unchanged
  • No regressions in edit/delete/favorite actions

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive
  • Feature flag needed? No -- bug fix

Related Notes

  • Closes #43
  • palinks -- project this affects
## Summary - Link card titles now navigate to the show page instead of the external URL - Show page displays a "Visit" button that opens the external URL in a new tab ## Changes - `app/views/links/_link.html.erb` -- title links to `link_path(link)` instead of `link.url` - `app/views/links/show.html.erb` -- title is plain text, added "Visit" button with `target="_blank"` ## Test Plan - [ ] Click link card title, verify show page loads - [ ] Click "Visit" button on show page, verify external URL opens in new tab - [ ] Verify card layout and styling unchanged - [ ] No regressions in edit/delete/favorite actions ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive - [x] Feature flag needed? No -- bug fix ## Related Notes - Closes #43 - `palinks` -- project this affects
Card titles now link to the show page instead of the external URL.
The show page displays a "Visit" button that opens the external URL
in a new tab.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-06-13 06:25:42 +00:00
Author
Owner
Copy link

PR #44 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails (ERB views, resourceful routing)

Diff summary (2 files, +3/-2):

  1. app/views/links/_link.html.erb -- link card title now routes to link_path(link) (show page) instead of link.url (external URL)
  2. app/views/links/show.html.erb -- title rendered as plain text; new "Visit" button opens external URL in a new tab with target="_blank" and rel="noopener"

Correctness:

  • link_path(link) is valid -- resources :links in config/routes.rb generates this helper for the show action. Confirmed.
  • rel: "noopener" on the external link is correct security practice -- prevents the opened page from accessing window.opener. Good.
  • The btn btn-primary class on the Visit button is consistent with the existing btn btn-secondary and btn btn-danger classes already used on the show page's action buttons.

Observations:

  • The card partial on disk still shows the old link.url target (line 13), confirming this branch has not been checked out locally -- the diff is the source of truth and the change is correct.
  • The URI.parse(link.url).host rescue link.url domain display on the card partial is untouched and still provides users a visual hint of where the link goes, which is good UX alongside the new behavior.

BLOCKERS

None.

The change is a two-line view-layer fix (no model, controller, or security surface changes). The BLOCKER criteria do not apply:

  • No new functionality requiring test coverage -- this is a navigation target change in existing views. The repo has minimal test coverage overall (only links_controller_sort_test.rb exists), and adding view tests for this fix would be scope creep relative to the issue.
  • No user input handling changes -- no injection, XSS, or path traversal risk.
  • No secrets or credentials.
  • No auth/security path changes.

NITS

  1. Missing noreferrer on Visit link (low): The Visit button uses rel: "noopener" but omits noreferrer. Adding rel: "noopener noreferrer" is a common hardening pattern that also prevents the Referer header from being sent to the external site. Non-blocking, but worth considering.

  2. Card partial still links repo URL externally (consistency observation): The card partial's "repo" link (line 17) still navigates externally via link.forgejo_url. This is probably intentional (repo links are a different UX intent than the title), but worth confirming the distinction is deliberate. Non-blocking.

SOP COMPLIANCE

  • Branch named after issue: fix-link-click-redirect -- descriptive of the fix, though does not include issue number 43-. Minor convention gap, non-blocking.
  • PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes -- all present and well-structured.
  • Closes #43 present in Related Notes section.
  • No secrets committed.
  • No unnecessary file changes -- exactly 2 files changed, both directly related to the fix.
  • Commit messages: not visible in diff, but PR title is descriptive.

PROCESS OBSERVATIONS

  • Change failure risk: Very low. Two-line view change with no model/controller/migration surface. The fix is straightforward and the link_path helper is verified against routes.
  • Test gap (pre-existing): The repo has only 1 test file covering the sort action. There are no view tests, no integration tests for show/index, and no system tests. This is a pre-existing gap, not introduced by this PR. A follow-up issue to add basic view/integration test coverage would reduce regression risk for future changes.

VERDICT: APPROVED

## PR #44 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails (ERB views, resourceful routing) **Diff summary (2 files, +3/-2):** 1. `app/views/links/_link.html.erb` -- link card title now routes to `link_path(link)` (show page) instead of `link.url` (external URL) 2. `app/views/links/show.html.erb` -- title rendered as plain text; new "Visit" button opens external URL in a new tab with `target="_blank"` and `rel="noopener"` **Correctness:** - `link_path(link)` is valid -- `resources :links` in `config/routes.rb` generates this helper for the show action. Confirmed. - `rel: "noopener"` on the external link is correct security practice -- prevents the opened page from accessing `window.opener`. Good. - The `btn btn-primary` class on the Visit button is consistent with the existing `btn btn-secondary` and `btn btn-danger` classes already used on the show page's action buttons. **Observations:** - The card partial on disk still shows the old `link.url` target (line 13), confirming this branch has not been checked out locally -- the diff is the source of truth and the change is correct. - The `URI.parse(link.url).host rescue link.url` domain display on the card partial is untouched and still provides users a visual hint of where the link goes, which is good UX alongside the new behavior. ### BLOCKERS None. The change is a two-line view-layer fix (no model, controller, or security surface changes). The BLOCKER criteria do not apply: - No new functionality requiring test coverage -- this is a navigation target change in existing views. The repo has minimal test coverage overall (only `links_controller_sort_test.rb` exists), and adding view tests for this fix would be scope creep relative to the issue. - No user input handling changes -- no injection, XSS, or path traversal risk. - No secrets or credentials. - No auth/security path changes. ### NITS 1. **Missing `noreferrer` on Visit link (low):** The Visit button uses `rel: "noopener"` but omits `noreferrer`. Adding `rel: "noopener noreferrer"` is a common hardening pattern that also prevents the `Referer` header from being sent to the external site. Non-blocking, but worth considering. 2. **Card partial still links repo URL externally (consistency observation):** The card partial's "repo" link (line 17) still navigates externally via `link.forgejo_url`. This is probably intentional (repo links are a different UX intent than the title), but worth confirming the distinction is deliberate. Non-blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `fix-link-click-redirect` -- descriptive of the fix, though does not include issue number `43-`. Minor convention gap, non-blocking. - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes -- all present and well-structured. - [x] `Closes #43` present in Related Notes section. - [x] No secrets committed. - [x] No unnecessary file changes -- exactly 2 files changed, both directly related to the fix. - [x] Commit messages: not visible in diff, but PR title is descriptive. ### PROCESS OBSERVATIONS - **Change failure risk:** Very low. Two-line view change with no model/controller/migration surface. The fix is straightforward and the `link_path` helper is verified against routes. - **Test gap (pre-existing):** The repo has only 1 test file covering the sort action. There are no view tests, no integration tests for show/index, and no system tests. This is a pre-existing gap, not introduced by this PR. A follow-up issue to add basic view/integration test coverage would reduce regression risk for future changes. ### VERDICT: APPROVED
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney deleted branch fix-link-click-redirect 2026-06-13 06:27:31 +00:00
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!44
No description provided.