Skip to content
This repository was archived by the owner on Aug 6, 2026. It is now read-only.

Don't navigate the current page on meta/ctrl row click - #1111

Merged
ericnewcomer merged 1 commit into
mainfrom
ericnewcomer/command-click
Aug 5, 2026
Merged

ericnewcomer merged 1 commit into
mainfrom
ericnewcomer/command-click

Conversation

@ericnewcomer

Copy link
Copy Markdown
Member

Summary

Command/ctrl-clicking a row in the new list components opened the row in a new tab as expected, but the original page also navigated. The row click handler fired the temba-row-click event before checking for meta/ctrl, and host pages navigate on that event unconditionally — so the host swapped the current page out from under the new tab.

Changes

  • src/list/ContentList.ts — handleRowClick now resolves the row href and checks for meta/ctrl first. When the click opens a new tab, the handler returns without firing RowClick (or Redirected), so host pages never see it. Plain clicks are unchanged: RowClick fires, followed by Redirected when the row has a safe href. All list subclasses (flows, contacts, messages, campaigns, triggers, broadcasts) share this handler.
  • test/temba-content-list.test.ts — extended the meta-click test to also assert temba-row-click does not fire, which is the assertion that would have caught this.

Behavior examples

Action Before After
Click a row Navigates in place Navigates in place (unchanged)
Meta/ctrl-click a row New tab opens and current page navigates New tab opens, current page stays put

Test plan

  • Full suite passes in the components container (2803 passed, 0 failed)
  • Meta-click test asserts window.open is called once and neither temba-redirected nor temba-row-click fires

@claude

claude Bot commented Aug 5, 2026 •

Copy link
Copy Markdown

Claude finished @ericnewcomer's task in 47s —— View job


Review: PR #1111 — Don't navigate the current page on meta/ctrl row click

  • Read handleRowClick and surrounding context
  • Check href resolution / meta-ctrl logic for correctness
  • Review test coverage
  • Post review

Verdict: LGTM. The fix is correct and minimal — it hoists href resolution above the meta/ctrl check and returns early without firing RowClick/Redirected, so hosts that navigate unconditionally on RowClick no longer swap the page out from under the new tab.

Correctness

  • The reordering preserves all prior behaviors: plain clicks still fire RowClick then Redirected (when a safe href exists); no-href rows still fire only RowClick; checkbox-cell clicks still bail early (that guard is untouched, above the new code).
  • isSafeHref still guards window.open, so the open-redirect protection is intact — good that getRowHref is now resolved once into a local rather than called twice.

Security / performance — no concerns. Same-origin guard unchanged; one fewer getRowHref call on the common path.

Test coverage — the extended meta-click test now asserts RowClick doesn't fire, which is exactly the regression that would have caught this. Adequate; the ctrl-key branch shares the same code path.

Minor observations (non-blocking, out of scope):

  • The intentional tradeoff is that meta/ctrl-click no longer fires RowClick at all — any host relying on it for analytics/tracking independent of navigation won't see it on a new-tab open. This is documented in the PR body and is the right call given hosts navigate on that event.
  • Middle-click (auxclick) isn't handled, so middle-clicking a row won't open a new tab. That's pre-existing behavior (rows are <tr>, not <a>), not something this PR needs to address.
    · ericnewcomer/command-click

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

📊 Coverage Report

📊 Coverage Report
==================
Lines:      77724/85644 (90.75%)
Functions:  2191/2499 (87.68%)
Branches:   10090/12303 (82.01%)
==================
✅ Statement Coverage: 90.75% (above 80% threshold)

🎉 Build passed: Statement coverage meets the 80% threshold!

Summary

  • Lines: 90.75%
  • Functions: 87.68%
  • Branches: 82.01%

Coverage Report

@ericnewcomer
ericnewcomer merged commit 8c5dd20 into main Aug 5, 2026
4 checks passed
@ericnewcomer
ericnewcomer deleted the ericnewcomer/command-click branch August 5, 2026 22:59
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant