Skip to content

fix(sc-45073): don't render active connection filter as a self-link#3443

Open
dcschreiber wants to merge 4 commits into
masterfrom
bug/sc-45073/secondary-text-title-link
Open

fix(sc-45073): don't render active connection filter as a self-link#3443
dcschreiber wants to merge 4 commits into
masterfrom
bug/sc-45073/secondary-text-title-link

Conversation

@dcschreiber

@dcschreiber dcschreiber commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Fixes sc-45073. Right-clicking the secondary text's title in the connections panel and choosing "Open Link in New Tab" duplicated the current two-panel page.

The title was being rendered as an 'a' tag.
The title (TextFilter) had an href set as the current page (/<ref>?with=<book>), so the new tab just reopened it.
Fix: render the title as a non-link (it doesn't function as a link), so right-click shows the normal page menu.

The connections-panel header shows a strip of recent filters (TextFilter
chips). The active chip's href is the current page (/<ref>?with=<book>),
so right-clicking the secondary text's title and choosing "Open Link in
New Tab" just duplicated the current two-panel view instead of doing
anything useful.

Render the active recent filter as a plain (non-link) element so
right-click shows the normal page menu. The other chips stay links —
opening a different connection in a new tab is legitimate.
@gitvelocity-reviewer

Copy link
Copy Markdown

📊 Code Quality Score: 1/100

Base Score 14 × ESF 0.1 = 1.4, rounded to 1

Category Score Factors
🔭 Scope 2/20 Single file, single component, minimal behavioral change
🏗️ Architecture 1/20 No architectural change; minor conditional rendering refactor within existing component
⚙️ Implementation 4/20 Clean extraction of inner JSX variable, simple conditional rendering; straightforward logic
⚠️ Risk 3/20 Low risk, easily reversible; minor accessibility concern with span replacing anchor; click handler removed for active state
✅ Quality 3/15 No tests added; explanatory comment present; new CSS class textFilterTitle not verified in diff
🔒 Perf / Security 1/5 Prevents unintended navigation (duplicate tab) behavior; minor UX improvement

Was this score accurate? 👍 Yes · 👎 No

Scored by GitVelocity · How are scores calculated?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts the connections panel UI so the active “recent” text filter title is no longer rendered as an anchor, preventing the browser’s “Open Link in New Tab” behavior from duplicating the current two-panel view.

Changes:

  • Add logic to render the active recent TextFilter as non-clickable content instead of an <a> tag.
  • Refactor the TextFilter render output to reuse a shared inner element for both linked and non-linked cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +140 to +142
return renderAsTitle ?
<span className="textFilterTitle">{inner}</span> :
<a href={url} onClick={this.handleClick}>{inner}</a>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcschreiber this seems logical to me

@dcschreiber dcschreiber Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — switched the wrapper to a < div > so it's valid HTML.

@saengel saengel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the feedback from Copilot, otherwise looks good.

dcschreiber and others added 2 commits July 8, 2026 12:48
A <span> cannot legally contain the block-level <div> in `inner`; browsers
reparent it, which can shift layout. Address Copilot/saengel review feedback
by using a <div> wrapper.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dcschreiber dcschreiber requested a review from saengel July 8, 2026 09:56
@dcschreiber dcschreiber requested review from saengel and removed request for saengel July 8, 2026 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants