Added seperate labels for PR made #165
Open
+304
−61
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
Reviewer's GuideRefactors Sequence diagram for fetching and displaying PR merged/closed labelssequenceDiagram
participant User
participant scrumHelper
participant GitHubAPI
User->>scrumHelper: Trigger PR display (e.g., open popup)
scrumHelper->>scrumHelper: Determine if PR is closed
alt PR is closed and within 14 days
scrumHelper->>GitHubAPI: fetch PR details (merged status)
GitHubAPI-->>scrumHelper: Return PR merged status
alt PR is merged
scrumHelper->>User: Show 'merged' label
else PR is closed but not merged
scrumHelper->>User: Show 'closed' label
end
else PR is open
scrumHelper->>User: Show 'open' label
end
Class diagram for updated PR label logic in scrumHelper.jsclassDiagram
class scrumHelper {
+allIncluded(outputTarget)
+getChromeData()
+getDaysBetween(start, end)
+fetchPrMergedStatus(owner, repo, number, headers)
+writeGithubIssuesPrs()
}
scrumHelper : +pr_merged_true_button
scrumHelper : +pr_merged_false_button
scrumHelper : +issue_opened_button
scrumHelper : +issue_closed_button
scrumHelper : +pr_unmerged_button
scrumHelper : +githubIssuesData
scrumHelper : +githubToken
scrumHelper : +intervalWriteGithubIssues
scrumHelper --> fetchPrMergedStatus : uses
scrumHelper --> getDaysBetween : uses
scrumHelper --> writeGithubIssuesPrs : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Preeti9764 - I've reviewed your changes - here's some feedback:
- I noticed references to pr_unmerged_button and pr_merged_button in writeGithubIssuesPrs, but only pr_merged_true_button and pr_merged_false_button are defined; please unify your button variable names or define the missing ones to avoid reference errors.
- Fetching each PR’s merged status sequentially in a loop can trigger rate limits and slow down the report—consider batching these requests (e.g. via the GraphQL API) or caching results to reduce API calls.
- Since setInterval doesn’t await promises, any errors in your async callback may be unhandled—wrap the interval handler in try/catch or switch to an async-aware scheduling approach.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- I noticed references to pr_unmerged_button and pr_merged_button in writeGithubIssuesPrs, but only pr_merged_true_button and pr_merged_false_button are defined; please unify your button variable names or define the missing ones to avoid reference errors.
- Fetching each PR’s merged status sequentially in a loop can trigger rate limits and slow down the report—consider batching these requests (e.g. via the GraphQL API) or caching results to reduce API calls.
- Since setInterval doesn’t await promises, any errors in your async callback may be unhandled—wrap the interval handler in try/catch or switch to an async-aware scheduling approach.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+705
to
+709
| function getDaysBetween(start, end) { | ||
| const d1 = new Date(start); | ||
| const d2 = new Date(end); | ||
| return Math.ceil((d2 - d1) / (1000 * 60 * 60 * 24)); | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
src/scripts/scrumHelper.js
Outdated
Comment on lines
712
to
722
| async function fetchPrMergedStatus(owner, repo, number, headers) { | ||
| const url = `https://api.github.com/repos/${owner}/${repo}/pulls/${number}`; | ||
| try { | ||
| const res = await fetch(url, { headers }); | ||
| if (!res.ok) return null; | ||
| const data = await res.json(); | ||
| return data.merged_at ? true : false; | ||
| } catch (e) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
src/scripts/scrumHelper.js
Outdated
| const res = await fetch(url, { headers }); | ||
| if (!res.ok) return null; | ||
| const data = await res.json(); | ||
| return data.merged_at ? true : false; |
There was a problem hiding this comment.
suggestion (code-quality): Avoid unneeded ternary statements (simplify-ternary)
Suggested change
| return data.merged_at ? true : false; | |
| return !!data.merged_at; |
Explanation
It is possible to simplify certain ternary statements into either use of an|| or !.This makes the code easier to read, since there is no conditional logic.
|
@Preeti9764 I only get the merged label on the generated report screen, but not the compose window in Gmail. |
@hpdang let me check this and make other changes too as suggested by sourcery- ai . Thanks for feedback! |
|
@hpdang I have made changes and this is my report in gmail inbox |
|
@Preeti9764 can we swap the merged and closed color? I think we should follow GitHub standard, purple for merged and red for closed |
|
Please use the colors as in the below table. In the example please also demonstrate that the draft stage is working as expected.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #160
📝 Summary of Changes
📸 Screenshots / Demo (if UI-related)
Add screenshots, video, or link to deployed preview if applicable
✅ Checklist
👀 Reviewer Notes
These labels are there for users made pr not for their reviewed prs.
Summary by Sourcery
Add separate labels for PRs based on open, merged, or closed status and fetch merged state via GitHub API for accurate labeling.
New Features:
Enhancements: