feat: Make PRs and PR numbers clickable with links to GitHub #273
+33
−12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideEnables PR numbers and titles in the scrum report to be clickable links that open the corresponding GitHub/GitLab PR in a new tab from the extension popup, including necessary DOM event handling, CSS tweaks, and updated Chrome extension permissions. Sequence diagram for opening PR links from the scrum report popupsequenceDiagram
actor User
participant PopupDOM as PopupScrumReportDOM
participant ClickHandler as PopupClickHandler
participant ChromeTabs as ChromeTabsAPI
participant Browser as BrowserNewTab
participant GitHost as GitHubOrGitLab
User->>PopupDOM: Click PR number or title link
PopupDOM->>ClickHandler: Capture click event (capturing phase)
ClickHandler->>ClickHandler: Check closest a inside scrumReport
alt Valid HTTP link
ClickHandler-->>PopupDOM: preventDefault and stopPropagation
ClickHandler->>ChromeTabs: tabs.create url
ChromeTabs->>Browser: Open new tab with PR URL
Browser->>GitHost: Request PR page
GitHost-->>Browser: Return PR HTML
Browser-->>User: Display PR details in new tab
else No or non HTTP href
ClickHandler-->>PopupDOM: Do nothing
end
Flow diagram for rendering and interacting with PR links in scrum reportflowchart TD
A[Fetch PR data from GitHub GitLab] --> B[ScrumHelperJS builds list item HTML]
B --> C{PR state}
C -->|draft open closed merged| D[Render PR number as a link<br/>href html_url<br/>target _blank<br/>contenteditable false]
D --> E[Render PR title as a link<br/>href html_url<br/>target _blank<br/>contenteditable false]
E --> F[Insert list item into scrumReport container]
F --> G[CSS sets scrumReport links as read only and unselectable]
G --> H[User clicks PR number or title]
H --> I[popup.js click listener on document<br/>filters for links inside scrumReport]
I --> J{href starts with http}
J -->|yes| K[Prevent default and stop propagation]
K --> L[chrome.tabs.create with PR URL]
L --> M[New browser tab opens PR page]
J -->|no| N[No special handling]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
bot
added
javascript
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The global
document.addEventListener('click', ..., true)withstopImmediatePropagation()may be heavier than needed; consider attaching the handler directly to#scrumReportor its container and avoidingstopImmediatePropagationunless you have a concrete conflict to resolve. - Setting
user-select: noneon#scrumReport aprevents users from copying PR titles/links; consider relaxing this (e.g., only disabling editing via CSS, not text selection) to preserve normal copy behavior. - For the new
target="_blank"links, consider addingrel="noopener noreferrer"to avoid giving the opened page access to thewindow.openerobject.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The global `document.addEventListener('click', ..., true)` with `stopImmediatePropagation()` may be heavier than needed; consider attaching the handler directly to `#scrumReport` or its container and avoiding `stopImmediatePropagation` unless you have a concrete conflict to resolve.
- Setting `user-select: none` on `#scrumReport a` prevents users from copying PR titles/links; consider relaxing this (e.g., only disabling editing via CSS, not text selection) to preserve normal copy behavior.
- For the new `target="_blank"` links, consider adding `rel="noopener noreferrer"` to avoid giving the opened page access to the `window.opener` object.
## Individual Comments
### Comment 1
<location> `src/scripts/popup.js:1603-1612` </location>
<code_context>
});
+
+ // Handle clicks on links within scrumReport to open in new tabs
+ document.addEventListener('click', function(e) {
+ const target = e.target.closest('a');
+ if (target && target.closest('#scrumReport')) {
+ e.preventDefault();
+ e.stopPropagation();
+ e.stopImmediatePropagation();
+ const href = target.getAttribute('href');
+ if (href && href.startsWith('http')) {
+ chrome.tabs.create({ url: href });
+ }
+ return false;
+ }
+ }, true); // Use capture phase to handle before contentEditable
}
\ No newline at end of file
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid attaching a new document-level click handler every time this code path runs.
Since this listener is defined in this scope, it will be re-registered each time this block runs, causing duplicate click handlers and repeated work. Please move the `addEventListener` to a place that runs once (e.g., module init) or add a guard so it’s only attached a single time.
</issue_to_address>
### Comment 2
<location> `src/scripts/scrumHelper.js:1466-1469` </location>
<code_context>
if (isDraft) {
- li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a>${showOpenLabel ? ' ' + pr_draft_button : ''}`;
+ li = `<li><i>(${project})</i> - Made PR <a href='${html_url}' target='_blank' contenteditable='false'>(#${number})</a> - <a href='${html_url}' target='_blank' contenteditable='false'>${title}</a>${showOpenLabel ? ' ' + pr_draft_button : ''}`;
if (showCommits && item._allCommits && item._allCommits.length && !isNewPR) {
log(`[PR DEBUG] Rendering commits for existing draft PR #${number}:`, item._allCommits);
</code_context>
<issue_to_address>
**🚨 issue (security):** Opening links with `target="_blank"` without `rel` attributes can introduce a tabnabbing risk.
Please add `rel="noopener noreferrer"` to these `target="_blank"` links so the new tab cannot access `window.opener` and to mitigate reverse tabnabbing, especially since these URLs point to external PR/MR pages.
</issue_to_address>
### Comment 3
<location> `src/index.css:61-64` </location>
<code_context>
color: #3f51b5;
}
+#scrumReport a {
+ -webkit-user-modify: read-only;
+ user-select: none;
+}
+
</code_context>
<issue_to_address>
**suggestion:** Using `user-select: none` on links under `#scrumReport` may be unnecessarily restrictive for users.
This prevents users from copying PR titles or URLs from the report, which is likely useful behavior. Since `contenteditable='false'` plus the JS handler already guard against edits, consider removing `user-select: none` here or applying it only to elements where accidental selection is actually a problem.
```suggestion
#scrumReport a {
-webkit-user-modify: read-only;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@vedansh-5 could you please review this pr ser? Thank |
Anshumancanrock
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📌 Fixes
Fixes #272
📝 Summary of Changes
Problem
The click event listener for handling links within
#scrumReportwas defined inside thevalidateOrgOnBlur()function. Since this function runs every time the organization input field loses focus, the event listener was being re-attached repeatedly, leading to:Solution
Moved the event listener registration from inside
validateOrgOnBlur()to theDOMContentLoadedhandler, ensuring it's attached only once when the page loads.The listener uses event delegation with
e.target.closest('a')to handle clicks on any link within#scrumReport, whether the link exists at page load or is dynamically generated later. This pattern is both efficient and works correctly for dynamic content.📸 Screenshots / Demo (if UI-related)
Recording.2025-12-12.152604.mp4
✅ Checklist