fix: external link confirm on shift click #2416
Conversation
Greptile SummaryThis PR fixes a bug where shift+clicking an external link in the markdown renderer would show the confirmation modal but then open the link as a regular navigation rather than in a new window. The fix introduces a
Confidence Score: 4/5The change is safe to merge — shift/ctrl/meta clicks will correctly route through the Electron window-open handler without a broken confirmation modal. The core shift-click fix is well-implemented and leverages the existing setWindowOpenHandler in the main process. The one open question is alt+click: excluding altKey from the guard means alt+clicks skip the confirmation and fall to native behavior, which browsers treat as a download rather than a new-window navigation, so the Electron setWindowOpenHandler may not intercept it. This is a minor edge case that does not affect the primary fix but could produce undefined behavior for that specific input. src/renderer/lib/ui/markdown-renderer.tsx — specifically the altKey exclusion in shouldConfirmExternalLinkClick.
|
| Filename | Overview |
|---|---|
| src/renderer/lib/ui/markdown-renderer.tsx | Adds shouldConfirmExternalLinkClick guard to bypass confirmation dialog for modifier-key clicks; alt+click falls through to native behavior which may not be caught by the Electron window-open handler. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User clicks external link] --> B{isHttp?}
B -- No --> C[Default browser behavior]
B -- Yes --> D{shouldConfirmExternalLinkClick?\nbutton===0 AND no modifier keys}
D -- Yes --> E[e.preventDefault]
E --> F[confirmOpenExternalLink\nshows modal]
F --> G{User confirms?}
G -- Yes --> H[rpc.app.openExternal\nopens in system browser]
G -- No --> I[Cancelled]
D -- No\nshift / ctrl / meta / alt click --> J[Default browser behavior\ntarget=_blank fires]
J --> K[Electron setWindowOpenHandler\ncaught by externalLinks.ts]
K --> L[shell.openExternal\nopens in system browser\nno confirmation]
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/renderer/lib/ui/markdown-renderer.tsx:120
The `altKey` case is meaningfully different from shift/ctrl/meta. In browsers, alt+click signals a download rather than a new window/tab, so it won't reliably trigger `setWindowOpenHandler` (which intercepts `window.open`/`target="_blank"` navigations). The result for alt+click is undefined: the confirmation modal is skipped, yet the link may not be routed through `shell.openExternal` either. Keeping the confirmation for alt+click — the same as any other non-power-user modifier — avoids this gap without affecting the shift-click fix being sought.
```suggestion
return event.button === 0 && !event.metaKey && !event.ctrlKey && !event.shiftKey;
```
Reviews (1): Last reviewed commit: "fix(renderer): allow shift-clicks on mar..." | Re-trigger Greptile
|
@janburzinski |
|
shift+click opened the modal but also just directly opened the website so this fixes that even when shift+click on a link it opens the modal first before opening the website instnatly (like we do for e.g. ctrl+click or normal click on links) i hope this makes sense what i just wrote :D |
977547c to
cd01505
Compare
Description
Screenshot/Recording (if applicable)
https://streamable.com/xptkdn
Checklist
messages and, when possible, the PR title