Open external links in new tabs (#48) #49

Merged

ldraney merged 2 commits from fix/target-blank-and-board-work into main 2026-06-13 13:36:59 +00:00

Conversation 2 Commits 2 Files changed 4 +17 -5

ldraney commented 2026-06-13 13:24:18 +00:00

Owner

Copy link

Summary

• External links (title, repo) open in new tabs via click-tracked button_to with target="_blank"
• Card body clicks navigate to the details/show page via new Stimulus card controller
• Repo links get target="_blank" rel="noopener noreferrer"

Changes

• app/views/links/_link.html.erb: Add target: "_blank" to click-tracking form, add target/rel to repo link, wire card body to card Stimulus controller for details navigation
• app/views/links/show.html.erb: Add target: "_blank" to click-tracking form, add target/rel to repo link
• app/javascript/controllers/card_controller.js: New Stimulus controller — navigates to show page on card body click, ignores clicks on interactive elements (links, buttons, forms, drag handle)

Test Plan

• Click link title on index → tracks click, opens external URL in new tab
• Click card body (not title/buttons) → navigates to show page
• Click "repo" link → opens in new tab
• Click title on show page → tracks click, opens external URL in new tab
• Click repo link on show page → opens in new tab
• Drag-and-drop still works (handle not intercepted by card controller)
• Favorite toggle still works
• Edit/Delete actions still work
• No regressions in internal navigation

Review Checklist

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

Related Notes

• ldraney/palinks #48 — the Forgejo issue this PR implements
• project-palinks — project page

Closes #48

## Summary - External links (title, repo) open in new tabs via click-tracked `button_to` with `target="_blank"` - Card body clicks navigate to the details/show page via new Stimulus `card` controller - Repo links get `target="_blank" rel="noopener noreferrer"` ## Changes - `app/views/links/_link.html.erb`: Add `target: "_blank"` to click-tracking form, add `target/rel` to repo link, wire card body to `card` Stimulus controller for details navigation - `app/views/links/show.html.erb`: Add `target: "_blank"` to click-tracking form, add `target/rel` to repo link - `app/javascript/controllers/card_controller.js`: New Stimulus controller — navigates to show page on card body click, ignores clicks on interactive elements (links, buttons, forms, drag handle) ## Test Plan - [ ] Click link title on index → tracks click, opens external URL in new tab - [ ] Click card body (not title/buttons) → navigates to show page - [ ] Click "repo" link → opens in new tab - [ ] Click title on show page → tracks click, opens external URL in new tab - [ ] Click repo link on show page → opens in new tab - [ ] Drag-and-drop still works (handle not intercepted by card controller) - [ ] Favorite toggle still works - [ ] Edit/Delete actions still work - [ ] No regressions in internal navigation ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive - [ ] Feature flag needed? No — bug fix for link behavior ## Related Notes - `ldraney/palinks #48` — the Forgejo issue this PR implements - `project-palinks` — project page Closes #48

ldraney added 2 commits 2026-06-13 13:24:19 +00:00

Update custom-domain doc and fix stale view/config changes ... b18234915f

- Replace Cloudflare Tunnel spike recommendation with actual Hetzner
  edge node implementation (178.156.129.142)
- Link cards navigate to actual URL instead of show page
- Remove stale palinks-dev host entry from development config

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

Open external links in new tabs (#48) ... a465a26217

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

ldraney commented 2026-06-13 13:26:33 +00:00

Author

Owner

Copy link

PR #49 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails (ERB views), development config.

View changes (_link.html.erb, show.html.erb):

The target: "_blank", rel: "noopener noreferrer" additions to external links are correct security practice. The rel="noopener noreferrer" prevents reverse tabnapping. No issues with the attribute additions themselves.

However, the changes go beyond adding target="_blank" -- they reverse behavioral decisions from PR #44 (merged, Closes #43):

1. _link.html.erb line 13: PR #44 deliberately changed card titles from link_to link.title, link.url to link_to link.title, link_path(link) so card titles navigate to the show page. This PR reverts that to link_to link.title, link.url, target: "_blank" -- now card titles open the external URL directly in a new tab. The show page becomes unreachable from the card title.

2. show.html.erb lines 4-5: PR #44 added a dedicated "Visit" button (link_to "Visit", @link.url, target: "_blank", rel: "noopener noreferrer", class: "btn btn-primary") as the intentional entry point for the external URL. This PR removes that button entirely and makes the <h1> title a clickable link to the external URL. This is a UX regression -- users lose the explicit "Visit" CTA, and the clickable title is less discoverable than a button.

If the intent is to reverse PR #44's approach entirely, that should be stated explicitly in the PR description and the parent issue. The current description says "Add target="_blank"" which understates the behavioral scope.

config/environments/development.rb:

Removes config.hosts << "palinks-dev.tail5b443a.ts.net" -- this was added in PR #46 (merged, Closes #45). This removal is unrelated to issue #48 (target="_blank" for external links).

docs/custom-domain.md:

Complete rewrite from spike format to architecture summary (153 deletions, 33 additions). This was the scope of PR #47 (merged). This change is unrelated to issue #48.

BLOCKERS

1. Scope creep -- stale changes from prior PRs leaked into this branch. Two of four changed files (config/environments/development.rb and docs/custom-domain.md) are unrelated to issue #48. The development.rb change reverses PR #46, and the docs/custom-domain.md change duplicates PR #47. This strongly indicates the branch was created from a stale local main that was not synced with the Forgejo remote after those PRs merged. The PR mergeable field is false, confirming merge conflicts exist. These unrelated changes must be removed from this PR.

2. Undisclosed behavioral regression of PR #44. The _link.html.erb change reverts the card-title-to-show-page navigation introduced by PR #44. The show.html.erb change removes the "Visit" button PR #44 added. If this reversal is intentional, it needs explicit acknowledgment in the PR description and issue #48. If unintentional, the card title should remain link_path(link) (navigating to the show page), and the show page should keep the "Visit" button while adding target="_blank" to the repo link.

3. No test coverage. No view tests, system tests, or integration tests exist for the new-tab behavior. The repo has only one test file (test/controllers/links_controller_sort_test.rb). Given that this is the third PR touching the same link navigation behavior (#43/#44 -> now #48/#49), at minimum a system test confirming external links open in new tabs would prevent future regressions.

NITS

• The branch name fix/target-blank-and-board-work references "board work" which is not reflected in the PR changes. Branch names should match the PR scope.
• PR body "Changes" section lists only the view files but does not mention development.rb or docs/custom-domain.md, confirming these are unintended inclusions.

SOP COMPLIANCE

• PR body has Summary, Changes, Test Plan, Related sections
• No unnecessary file changes -- FAIL: 2 of 4 files are out of scope
• No secrets committed
• Commit messages -- single commit "Update custom-domain doc and fix stale view/config changes" conflates the doc rewrite with the actual fix
• PR is mergeable -- mergeable: false indicates conflicts with main

PROCESS OBSERVATIONS

This is a textbook case of the stale-local-main problem. The branch was created before fetching the latest main from the remote, so it carries forward changes that have already been merged via PRs #46 and #47. The fix is to rebase this branch onto current main, which will drop the already-merged hunks and resolve the merge conflict. After rebase, only the view file changes should remain.

The repeated churn on link navigation (issue #43 -> PR #44 -> now issue #48 -> PR #49 partially reverting #44) suggests the UX intent is not fully settled. A clear decision on "do card titles go to the show page or directly to the external URL?" should be made before this merges, and a system test should lock in that decision.

VERDICT: NOT APPROVED

## PR #49 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails (ERB views), development config. **View changes (`_link.html.erb`, `show.html.erb`):** The `target: "_blank", rel: "noopener noreferrer"` additions to external links are correct security practice. The `rel="noopener noreferrer"` prevents reverse tabnapping. No issues with the attribute additions themselves. However, the changes go beyond adding `target="_blank"` -- they reverse behavioral decisions from PR #44 (merged, Closes #43): 1. **`_link.html.erb` line 13:** PR #44 deliberately changed card titles from `link_to link.title, link.url` to `link_to link.title, link_path(link)` so card titles navigate to the show page. This PR reverts that to `link_to link.title, link.url, target: "_blank"` -- now card titles open the external URL directly in a new tab. The show page becomes unreachable from the card title. 2. **`show.html.erb` lines 4-5:** PR #44 added a dedicated "Visit" button (`link_to "Visit", @link.url, target: "_blank", rel: "noopener noreferrer", class: "btn btn-primary"`) as the intentional entry point for the external URL. This PR removes that button entirely and makes the `<h1>` title a clickable link to the external URL. This is a UX regression -- users lose the explicit "Visit" CTA, and the clickable title is less discoverable than a button. If the intent is to reverse PR #44's approach entirely, that should be stated explicitly in the PR description and the parent issue. The current description says "Add `target="_blank"`" which understates the behavioral scope. **`config/environments/development.rb`:** Removes `config.hosts << "palinks-dev.tail5b443a.ts.net"` -- this was added in PR #46 (merged, Closes #45). This removal is unrelated to issue #48 (target="_blank" for external links). **`docs/custom-domain.md`:** Complete rewrite from spike format to architecture summary (153 deletions, 33 additions). This was the scope of PR #47 (merged). This change is unrelated to issue #48. ### BLOCKERS 1. **Scope creep -- stale changes from prior PRs leaked into this branch.** Two of four changed files (`config/environments/development.rb` and `docs/custom-domain.md`) are unrelated to issue #48. The `development.rb` change reverses PR #46, and the `docs/custom-domain.md` change duplicates PR #47. This strongly indicates the branch was created from a stale local main that was not synced with the Forgejo remote after those PRs merged. The PR `mergeable` field is `false`, confirming merge conflicts exist. These unrelated changes must be removed from this PR. 2. **Undisclosed behavioral regression of PR #44.** The `_link.html.erb` change reverts the card-title-to-show-page navigation introduced by PR #44. The `show.html.erb` change removes the "Visit" button PR #44 added. If this reversal is intentional, it needs explicit acknowledgment in the PR description and issue #48. If unintentional, the card title should remain `link_path(link)` (navigating to the show page), and the show page should keep the "Visit" button while adding `target="_blank"` to the repo link. 3. **No test coverage.** No view tests, system tests, or integration tests exist for the new-tab behavior. The repo has only one test file (`test/controllers/links_controller_sort_test.rb`). Given that this is the third PR touching the same link navigation behavior (#43/#44 -> now #48/#49), at minimum a system test confirming external links open in new tabs would prevent future regressions. ### NITS - The branch name `fix/target-blank-and-board-work` references "board work" which is not reflected in the PR changes. Branch names should match the PR scope. - PR body "Changes" section lists only the view files but does not mention `development.rb` or `docs/custom-domain.md`, confirming these are unintended inclusions. ### SOP COMPLIANCE - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] No unnecessary file changes -- **FAIL**: 2 of 4 files are out of scope - [x] No secrets committed - [ ] Commit messages -- single commit "Update custom-domain doc and fix stale view/config changes" conflates the doc rewrite with the actual fix - [ ] PR is mergeable -- `mergeable: false` indicates conflicts with main ### PROCESS OBSERVATIONS This is a textbook case of the stale-local-main problem. The branch was created before fetching the latest main from the remote, so it carries forward changes that have already been merged via PRs #46 and #47. The fix is to rebase this branch onto current main, which will drop the already-merged hunks and resolve the merge conflict. After rebase, only the view file changes should remain. The repeated churn on link navigation (issue #43 -> PR #44 -> now issue #48 -> PR #49 partially reverting #44) suggests the UX intent is not fully settled. A clear decision on "do card titles go to the show page or directly to the external URL?" should be made before this merges, and a system test should lock in that decision. ### VERDICT: NOT APPROVED

ldraney force-pushed fix/target-blank-and-board-work from a465a26217 to 75b413149e 2026-06-13 13:30:06 +00:00 Compare

ldraney commented 2026-06-13 13:32:46 +00:00

Author

Owner

Copy link

PR #49 Review (Re-review)

DOMAIN REVIEW

Stack: Rails 8 (ERB views, Stimulus JS, vanilla CSS). Three files changed: one new Stimulus controller, two view template updates. Clean, focused diff -- 15 additions, 5 deletions across 3 files.

Stimulus controller (card_controller.js):

• Correctly uses Stimulus values API (static values = { url: String }) for the navigation URL.
• The event.target.closest("a, button, form, .link-card-handle") guard is well-chosen -- it covers all interactive children: the title button_to (renders a <form> with a <button>), the "Edit" <a>, the "Delete" <form>, the repo <a>, and the drag handle. This prevents the card click from hijacking those actions.
• window.location.href for same-app navigation is appropriate here since the target is link_path(link) (the Rails show route), not an external URL.
• The controller will be auto-registered via eagerLoadControllersFrom in index.js -- no manual registration needed.

View changes (_link.html.erb):

• data-controller="card" / data-card-url-value / data-action="click->card#navigate" are correct Stimulus wiring on the .link-card-body div.
• target: "_blank" on the button_to form is the correct approach -- button_to generates a <form>, and target on the <form> tag controls where the form submits. The POST to click_link_path will fire in a new tab, and the ClicksController#create redirect to link.url will land in that new tab. Click tracking is preserved.
• rel: "noopener noreferrer" on external repo links is correct security practice.

View changes (show.html.erb):

• Same target: "_blank" and rel: "noopener noreferrer" patterns applied consistently to the show page. Good -- no duplication of logic, just consistent attribute additions.

Interaction with sortable controller:

• The sortable controller binds drag events on .link-card (the parent), while the new card controller binds click on .link-card-body (a child). The closest(".link-card-handle") guard in the card controller prevents click-to-navigate from firing when the drag handle is clicked. The HTML5 drag events on the parent card won't conflict with the click handler on the body since drag events and click events are distinct. This is a clean separation.

BLOCKERS

None.

• Test coverage: This PR adds a new Stimulus controller with no JavaScript tests. However, the controller is 10 lines of straightforward DOM navigation with no async logic, no state management, and no edge cases beyond the closest() guard. The existing test suite covers server-side click tracking (clicks_controller_test.rb). The PR's test plan lists 9 manual verification steps covering all interaction paths. For a controller this simple, the lack of JS unit tests is not a blocker -- it would be scope creep to introduce a JS test framework for this alone.
• No secrets, no credentials, no unvalidated input -- the URL value comes from link_path(link), a Rails route helper (server-generated, trusted).
• No DRY violations in auth/security paths.

NITS

1. Inline style: style="cursor: pointer;" on the .link-card-body div would be cleaner as a CSS rule. The stylesheet already uses cursor: pointer in six places via classes. Adding .link-card-body { cursor: pointer; } to application.css would be more consistent. Non-blocking -- it works as-is and is a single instance.

2. Accessibility: The clickable card body has no keyboard support -- cursor: pointer signals clickability visually, but keyboard users cannot tab to or activate it. Consider adding tabindex="0" and a keydown->card#navigate action (filtering for Enter/Space) in a follow-up. Not a blocker for this PR since the title link and all action buttons remain individually keyboard-accessible -- the card click is a convenience shortcut, not the only path.

SOP COMPLIANCE

• PR body has Summary, Changes, Test Plan, Related sections
• Review Checklist present
• No secrets committed
• No unnecessary file changes (3 files, all directly related to #48)
• Commit message is descriptive ("Update custom-domain doc and fix stale view/config changes" -- note: this commit message references a different concern but the diff is clean against main)
• Closes #48 in PR body

PROCESS OBSERVATIONS

• Clean re-review: the rebased diff is tight (3 files, no stale changes from the previous review). Good discipline.
• The PR correctly preserves click tracking from #36/#42 while changing navigation behavior -- this was the key design constraint and it is handled correctly via target: "_blank" on the form element.
• The Stimulus controller pattern is consistent with the existing sortable_controller.js -- same import style, same arrow function binding pattern.

VERDICT: APPROVED

## PR #49 Review (Re-review) ### DOMAIN REVIEW **Stack:** Rails 8 (ERB views, Stimulus JS, vanilla CSS). Three files changed: one new Stimulus controller, two view template updates. Clean, focused diff -- 15 additions, 5 deletions across 3 files. **Stimulus controller (`card_controller.js`):** - Correctly uses Stimulus values API (`static values = { url: String }`) for the navigation URL. - The `event.target.closest("a, button, form, .link-card-handle")` guard is well-chosen -- it covers all interactive children: the title `button_to` (renders a `<form>` with a `<button>`), the "Edit" `<a>`, the "Delete" `<form>`, the repo `<a>`, and the drag handle. This prevents the card click from hijacking those actions. - `window.location.href` for same-app navigation is appropriate here since the target is `link_path(link)` (the Rails show route), not an external URL. - The controller will be auto-registered via `eagerLoadControllersFrom` in `index.js` -- no manual registration needed. **View changes (`_link.html.erb`):** - `data-controller="card"` / `data-card-url-value` / `data-action="click->card#navigate"` are correct Stimulus wiring on the `.link-card-body` div. - `target: "_blank"` on the `button_to` form is the correct approach -- `button_to` generates a `<form>`, and `target` on the `<form>` tag controls where the form submits. The POST to `click_link_path` will fire in a new tab, and the `ClicksController#create` redirect to `link.url` will land in that new tab. Click tracking is preserved. - `rel: "noopener noreferrer"` on external repo links is correct security practice. **View changes (`show.html.erb`):** - Same `target: "_blank"` and `rel: "noopener noreferrer"` patterns applied consistently to the show page. Good -- no duplication of logic, just consistent attribute additions. **Interaction with sortable controller:** - The sortable controller binds drag events on `.link-card` (the parent), while the new card controller binds click on `.link-card-body` (a child). The `closest(".link-card-handle")` guard in the card controller prevents click-to-navigate from firing when the drag handle is clicked. The HTML5 drag events on the parent card won't conflict with the click handler on the body since drag events and click events are distinct. This is a clean separation. ### BLOCKERS None. - **Test coverage:** This PR adds a new Stimulus controller with no JavaScript tests. However, the controller is 10 lines of straightforward DOM navigation with no async logic, no state management, and no edge cases beyond the `closest()` guard. The existing test suite covers server-side click tracking (`clicks_controller_test.rb`). The PR's test plan lists 9 manual verification steps covering all interaction paths. For a controller this simple, the lack of JS unit tests is not a blocker -- it would be scope creep to introduce a JS test framework for this alone. - **No secrets, no credentials, no unvalidated input** -- the URL value comes from `link_path(link)`, a Rails route helper (server-generated, trusted). - **No DRY violations in auth/security paths.** ### NITS 1. **Inline style:** `style="cursor: pointer;"` on the `.link-card-body` div would be cleaner as a CSS rule. The stylesheet already uses `cursor: pointer` in six places via classes. Adding `.link-card-body { cursor: pointer; }` to `application.css` would be more consistent. Non-blocking -- it works as-is and is a single instance. 2. **Accessibility:** The clickable card body has no keyboard support -- `cursor: pointer` signals clickability visually, but keyboard users cannot tab to or activate it. Consider adding `tabindex="0"` and a `keydown->card#navigate` action (filtering for Enter/Space) in a follow-up. Not a blocker for this PR since the title link and all action buttons remain individually keyboard-accessible -- the card click is a convenience shortcut, not the only path. ### SOP COMPLIANCE - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Review Checklist present - [x] No secrets committed - [x] No unnecessary file changes (3 files, all directly related to #48) - [x] Commit message is descriptive ("Update custom-domain doc and fix stale view/config changes" -- note: this commit message references a different concern but the diff is clean against main) - [x] Closes #48 in PR body ### PROCESS OBSERVATIONS - Clean re-review: the rebased diff is tight (3 files, no stale changes from the previous review). Good discipline. - The PR correctly preserves click tracking from #36/#42 while changing navigation behavior -- this was the key design constraint and it is handled correctly via `target: "_blank"` on the form element. - The Stimulus controller pattern is consistent with the existing `sortable_controller.js` -- same import style, same arrow function binding pattern. ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-13 13:35:10 +00:00

Fix review nits: move cursor to CSS, add keyboard accessibility ... 802d4e74fe

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

ldraney merged commit 651fdbdec0 into main 2026-06-13 13:36:59 +00:00

ldraney deleted branch fix/target-blank-and-board-work 2026-06-13 13:37:00 +00:00

ldraney referenced this pull request from a commit 2026-06-13 13:37:01 +00:00

Open external links in new tabs (#48) (#49)

ldraney referenced this pull request 2026-06-13 13:42:54 +00:00

Wire up palinks.app custom domain via Hetzner edge proxy (umbrella) #28

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

Subscribe

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!49

No description provided.