Skip to content

feat: open links in new tab#76

Open
DarshanCode2005 wants to merge 1 commit intohuggingface:mainfrom
DarshanCode2005:link-new-tab
Open

feat: open links in new tab#76
DarshanCode2005 wants to merge 1 commit intohuggingface:mainfrom
DarshanCode2005:link-new-tab

Conversation

@DarshanCode2005
Copy link
Copy Markdown

@DarshanCode2005 DarshanCode2005 commented Apr 23, 2026

Closes #64

Links rendered via ReactMarkdown inside chat messages were opening in the same tab, disrupting the user’s session and navigation flow.

Solution

  • Added a custom a component renderer in MarkdownContent.tsx

  • Ensured all markdown links include:

    • target="_blank"
    • rel="noopener noreferrer"

Impact

  • Prevents chat session disruption when users click on links
  • Improves overall UX consistency across the application

Additional Context

Other link surfaces -- ClaudeCapDialog, WelcomeScreen, and ToolCallGroup already had target="_blank" configured correctly.
This change ensures consistent behavior across all markdown-rendered content, including agent-generated messages.

Testing

  • Verified that all links rendered via markdown now open in a new tab
  • Confirmed no regression in existing link behaviors across other components

@akseljoonas
Copy link
Copy Markdown
Collaborator

@claude please review this PR. Focus on correctness, security, LiteLLM/Bedrock routing impact, and whether it's safe to merge against current main. Keep it concise and prefer inline comments where it matters.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Claude finished @akseljoonas's task in 57s —— View job


PR Review: feat: open links in new tab

  • Gather context
  • Read changed file and diff
  • Analyze correctness, security, and routing impact
  • Post final review

Verdict: Safe to merge. This is a minimal, well-scoped frontend change.

Correctness

The implementation is correct:

  • useMemo with an empty dependency array ([]) is appropriate — the component factory has no dependencies on outer scope variables that change, so it won't cause stale closure bugs.
  • ComponentPropsWithoutRef<'a'> is the right type for a React DOM component override in react-markdown.
  • The remarkPlugins memoization was already in place, so this follows the existing pattern consistently.

Security

rel="noopener noreferrer" is correctly present, which prevents tab-napping (the opened page can't access window.opener).

Minor (non-blocking): The {...props} spread is placed after target and rel:

// MarkdownContent.tsx:171
<a href={href} target="_blank" rel="noopener noreferrer" {...props}>

In JSX, later attributes win, so if ...props ever contains target or rel, they would silently override the security values. In practice, react-markdown does not inject these attributes for standard [text](url) links, so there's no actual risk here. A more defensive pattern would be:

<a href={href} {...props} target="_blank" rel="noopener noreferrer">

This guarantees the security attributes are never overridden regardless of what react-markdown's internals pass through. Fix this →

LiteLLM / Bedrock Routing Impact

None. This change is entirely in the frontend render layer — no API calls, no model routing, no backend logic touched.

Merge Safety vs Current Main

No conflicts, no regressions. The PR body correctly notes that ClaudeCapDialog, WelcomeScreen, and ToolCallGroup already had target="_blank" — this brings MarkdownContent into consistency with those components.

The prop-ordering note above is optional hardening; it does not block merge.

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.

open links in new tab

2 participants