-
Notifications
You must be signed in to change notification settings - Fork 86
Fetch all github activities #168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR removes the hardcoded default organization, enables fetching GitHub activities across all public repositories when no org is specified, and instruments the fetching and rendering logic with enhanced cache-key handling and debug logging. Sequence diagram for fetching GitHub activities with or without organizationsequenceDiagram
actor User
participant Popup as Popup UI
participant Storage as chrome.storage.local
participant ScrumHelper as scrumHelper.js
participant GitHub as GitHub API
User->>Popup: Enter GitHub username and (optionally) org
Popup->>Storage: Save orgName (can be empty)
User->>Popup: Click Generate Report
Popup->>ScrumHelper: Trigger allIncluded('popup')
ScrumHelper->>Storage: Get orgName, username, dates
ScrumHelper->>ScrumHelper: Build cacheKey (includes orgName or 'all')
ScrumHelper->>GitHub: Fetch issues & PRs (with orgName if set, else all public)
GitHub-->>ScrumHelper: Return issues & PRs
ScrumHelper->>ScrumHelper: Process and cache data
ScrumHelper->>Popup: Render report in popup
Popup->>User: Show generated report
Class diagram for updated scrumHelper.js data flowclassDiagram
class ScrumHelper {
- orgName: string
- githubUsername: string
- githubToken: string
- startingDate: string
- endingDate: string
- githubCache: object
+ allIncluded(outputTarget)
+ fetchGithubData()
+ processGithubData(data)
+ writeScrumBody()
+ writeGithubPrsReviews()
+ triggerScrumGeneration()
}
class chrome.storage.local {
+ get(keys, callback)
+ set(items, callback)
}
class GitHubAPI {
+ searchIssues(query)
+ getUser(username)
}
ScrumHelper --> "1" chrome.storage.local : uses
ScrumHelper --> "1" GitHubAPI : fetches data from
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Preeti9764 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/scripts/scrumHelper.js:365` </location>
<code_context>
- let issueUrl = `https://api.github.com/search/issues?q=author%3A${githubUsername}+org%3A${orgName}+created%3A${startingDate}..${endingDate}&per_page=100`;
- let prUrl = `https://api.github.com/search/issues?q=commenter%3A${githubUsername}+org%3A${orgName}+updated%3A${startingDate}..${endingDate}&per_page=100`;
+ // Build org part for query only if orgName is set and not empty
+ console.log('[SCRUM-HELPER] orgName before API query:', orgName);
+ let orgPart = orgName && orgName.trim() ? `+org%3A${orgName}` : '';
+ console.log('[SCRUM-HELPER] orgPart for API:', orgPart);
+ let issueUrl = `https://api.github.com/search/issues?q=author%3A${githubUsername}${orgPart}+created%3A${startingDate}..${endingDate}&per_page=100`;
+ let prUrl = `https://api.github.com/search/issues?q=commenter%3A${githubUsername}${orgPart}+updated%3A${startingDate}..${endingDate}&per_page=100`;
+ console.log('[SCRUM-HELPER] issueUrl:', issueUrl);
+ console.log('[SCRUM-HELPER] prUrl:', prUrl);
</code_context>
<issue_to_address>
Query construction for orgName may result in malformed queries if orgName contains special characters.
URL-encode orgName before adding it to the query string to prevent API errors from special characters.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Build org part for query only if orgName is set and not empty
console.log('[SCRUM-HELPER] orgName before API query:', orgName);
let orgPart = orgName && orgName.trim() ? `+org%3A${orgName}` : '';
console.log('[SCRUM-HELPER] orgPart for API:', orgPart);
let issueUrl = `https://api.github.com/search/issues?q=author%3A${githubUsername}${orgPart}+created%3A${startingDate}..${endingDate}&per_page=100`;
let prUrl = `https://api.github.com/search/issues?q=commenter%3A${githubUsername}${orgPart}+updated%3A${startingDate}..${endingDate}&per_page=100`;
console.log('[SCRUM-HELPER] issueUrl:', issueUrl);
console.log('[SCRUM-HELPER] prUrl:', prUrl);
=======
// Build org part for query only if orgName is set and not empty, and URL-encode orgName
console.log('[SCRUM-HELPER] orgName before API query:', orgName);
let encodedOrgName = orgName && orgName.trim() ? encodeURIComponent(orgName) : '';
let orgPart = encodedOrgName ? `+org%3A${encodedOrgName}` : '';
console.log('[SCRUM-HELPER] orgPart for API:', orgPart);
let issueUrl = `https://api.github.com/search/issues?q=author%3A${githubUsername}${orgPart}+created%3A${startingDate}..${endingDate}&per_page=100`;
let prUrl = `https://api.github.com/search/issues?q=commenter%3A${githubUsername}${orgPart}+updated%3A${startingDate}..${endingDate}&per_page=100`;
console.log('[SCRUM-HELPER] issueUrl:', issueUrl);
console.log('[SCRUM-HELPER] prUrl:', prUrl);
>>>>>>> REPLACE
</suggested_fix>
Help me be more useful! Please click π or π on each comment and I'll use the feedback to improve your reviews.
src/scripts/scrumHelper.js
Outdated
// Build org part for query only if orgName is set and not empty | ||
console.log('[SCRUM-HELPER] orgName before API query:', orgName); | ||
let orgPart = orgName && orgName.trim() ? `+org%3A${orgName}` : ''; | ||
console.log('[SCRUM-HELPER] orgPart for API:', orgPart); | ||
let issueUrl = `https://api.github.com/search/issues?q=author%3A${githubUsername}${orgPart}+created%3A${startingDate}..${endingDate}&per_page=100`; | ||
let prUrl = `https://api.github.com/search/issues?q=commenter%3A${githubUsername}${orgPart}+updated%3A${startingDate}..${endingDate}&per_page=100`; | ||
console.log('[SCRUM-HELPER] issueUrl:', issueUrl); | ||
console.log('[SCRUM-HELPER] prUrl:', prUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Query construction for orgName may result in malformed queries if orgName contains special characters.
URL-encode orgName before adding it to the query string to prevent API errors from special characters.
// Build org part for query only if orgName is set and not empty | |
console.log('[SCRUM-HELPER] orgName before API query:', orgName); | |
let orgPart = orgName && orgName.trim() ? `+org%3A${orgName}` : ''; | |
console.log('[SCRUM-HELPER] orgPart for API:', orgPart); | |
let issueUrl = `https://api.github.com/search/issues?q=author%3A${githubUsername}${orgPart}+created%3A${startingDate}..${endingDate}&per_page=100`; | |
let prUrl = `https://api.github.com/search/issues?q=commenter%3A${githubUsername}${orgPart}+updated%3A${startingDate}..${endingDate}&per_page=100`; | |
console.log('[SCRUM-HELPER] issueUrl:', issueUrl); | |
console.log('[SCRUM-HELPER] prUrl:', prUrl); | |
// Build org part for query only if orgName is set and not empty, and URL-encode orgName | |
console.log('[SCRUM-HELPER] orgName before API query:', orgName); | |
let encodedOrgName = orgName && orgName.trim() ? encodeURIComponent(orgName) : ''; | |
let orgPart = encodedOrgName ? `+org%3A${encodedOrgName}` : ''; | |
console.log('[SCRUM-HELPER] orgPart for API:', orgPart); | |
let issueUrl = `https://api.github.com/search/issues?q=author%3A${githubUsername}${orgPart}+created%3A${startingDate}..${endingDate}&per_page=100`; | |
let prUrl = `https://api.github.com/search/issues?q=commenter%3A${githubUsername}${orgPart}+updated%3A${startingDate}..${endingDate}&per_page=100`; | |
console.log('[SCRUM-HELPER] issueUrl:', issueUrl); | |
console.log('[SCRUM-HELPER] prUrl:', prUrl); |
test update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Preeti9764 - I've reviewed your changes and they look great!
Blocking issues:
- User controlled data in methods like
innerHTML
,outerHTML
ordocument.write
is an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
scrumReport.innerHTML
is an anti-pattern that can lead to XSS vulnerabilities (link)
Prompt for AI Agents
Please address the comments from this code review:
## Security Issues
### Issue 1
<location> `src/scripts/scrumHelper.js:496` </location>
<issue_to_address>
**security (opengrep-rules.javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Issue 2
<location> `src/scripts/scrumHelper.js:496` </location>
<issue_to_address>
**security (opengrep-rules.javascript.browser.security.insecure-innerhtml):** User controlled data in a `scrumReport.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
Help me be more useful! Please click π or π on each comment and I'll use the feedback to improve your reviews.
src/scripts/scrumHelper.js
Outdated
if (scrumReport) { | ||
let errorMsg = 'An error occurred while generating the report.'; | ||
if (err) { | ||
if (typeof err === 'string') errorMsg = err; | ||
else if (err.message) errorMsg = err.message; | ||
else errorMsg = JSON.stringify(err) | ||
} | ||
|
||
scrumReport.innerHTML = `<div class="error-message" style="color: #dc2626; font-weight: bold; padding: 10px;">${err.message || 'An error occurred while generating the report.'}</div>`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
security (opengrep-rules.javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML
, outerHTML
or document.write
is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
src/scripts/scrumHelper.js
Outdated
if (scrumReport) { | ||
let errorMsg = 'An error occurred while generating the report.'; | ||
if (err) { | ||
if (typeof err === 'string') errorMsg = err; | ||
else if (err.message) errorMsg = err.message; | ||
else errorMsg = JSON.stringify(err) | ||
} | ||
|
||
scrumReport.innerHTML = `<div class="error-message" style="color: #dc2626; font-weight: bold; padding: 10px;">${err.message || 'An error occurred while generating the report.'}</div>`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
security (opengrep-rules.javascript.browser.security.insecure-innerhtml): User controlled data in a scrumReport.innerHTML
is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
@hpdang @vedansh-5 Please review this and share your feedback, it will more enhanced as the changes of pr#164 got merged .Thanks! |
Sure, I will make a review soon, I am currently facing rate limitation from github.com due to using public requests while testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, thank you.
src/scripts/scrumHelper.js
Outdated
@@ -335,6 +349,7 @@ function allIncluded(outputTarget = 'email') { | |||
username: githubUsername, | |||
startDate: startingDate, | |||
endDate: endingDate, | |||
orgName: orgName || 'all' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different API calls are being made for fetching all of the data. Since a user could have contributed to a number of orgs, these different api calls will result in public api limit exemption.
We can have this feature as a token-only feature to not let it affect the primary functionality of out behavior.
In the current implementation, the limit would be reached soon if we are using public requests.
@hpdang Please share your views on this.
Suggestion - Even while making auth requests we can use graphQL to fetch all the contributions in one request and, and have the current issueUrls and prReviewedUrls for public requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vedansh-5 this request are for the different labels of pr, not for the github actvities fetched. I hope this clears you confusion .Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vedansh-5 this request are for the different labels of pr, not for the github actvities fetched. I hope this clears you confusion .Thanks!
Thanks for clearing it out!
src/scripts/scrumHelper.js
Outdated
console.log('[SCRUM-HELPER] orgName before API query:', orgName); | ||
console.log('[SCRUM-HELPER] orgName type:', typeof orgName); | ||
console.log('[SCRUM-HELPER] orgName length:', orgName ? orgName.length : 0); | ||
let orgPart = orgName && orgName.trim() ? `+org%3A${orgName}` : ''; | ||
console.log('[SCRUM-HELPER] orgPart for API:', orgPart); | ||
console.log('[SCRUM-HELPER] orgPart length:', orgPart.length); | ||
let issueUrl = `https://api.github.com/search/issues?q=author%3A${githubUsername}${orgPart}+created%3A${startingDate}..${endingDate}&per_page=100`; | ||
let prUrl = `https://api.github.com/search/issues?q=commenter%3A${githubUsername}${orgPart}+updated%3A${startingDate}..${endingDate}&per_page=100`; | ||
console.log('[SCRUM-HELPER] issueUrl:', issueUrl); | ||
console.log('[SCRUM-HELPER] prUrl:', prUrl); | ||
let userUrl = `https://api.github.com/users/${githubUsername}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even after clearing the orgName, the data is fetched for the previously set org even after the org is cleared.
It generates the correct data only after hitting the generate button again.
allRev1.mp4
@Preeti9764 this is the next thing we should have |
@hpdang Made the changes , Please have a look whenever you get time.Thanks! |
@vedansh-5 Share your views. thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! Thanks @Preeti9764
π Fixes
Fixes #142
π Summary of Changes
πΈ Screenshots / Demo (if UI-related)
β Checklist
Summary by Sourcery
Allow fetching GitHub activities from all public repositories by making the organization filter optional, updating cache keys and query construction accordingly, and enhancing the popup input handling and debug logging
New Features:
Enhancements:
org:
filter in GitHub API queries when the organization name is empty