Conversation
|
@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 finished @akseljoonas's task in 57s —— View job PR Review: feat: open links in new tab
Verdict: Safe to merge. This is a minimal, well-scoped frontend change. CorrectnessThe implementation is correct:
Security
Minor (non-blocking): The // MarkdownContent.tsx:171
<a href={href} target="_blank" rel="noopener noreferrer" {...props}>In JSX, later attributes win, so if <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 ImpactNone. This change is entirely in the frontend render layer — no API calls, no model routing, no backend logic touched. Merge Safety vs Current MainNo conflicts, no regressions. The PR body correctly notes that The prop-ordering note above is optional hardening; it does not block merge. |
Closes #64
Links rendered via
ReactMarkdowninside chat messages were opening in the same tab, disrupting the user’s session and navigation flow.Solution
Added a custom
acomponent renderer inMarkdownContent.tsxEnsured all markdown links include:
target="_blank"rel="noopener noreferrer"Impact
Additional Context
Other link surfaces --
ClaudeCapDialog,WelcomeScreen, andToolCallGroupalready hadtarget="_blank"configured correctly.This change ensures consistent behavior across all markdown-rendered content, including agent-generated messages.
Testing