-
Notifications
You must be signed in to change notification settings - Fork 100
Minor: using graphQL for commit fetching solving the api exemption issue. #147
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
Conversation
Signed-off-by: Vedansh Saini <[email protected]>
Reviewer's GuideThis PR introduces a GraphQL‐based batch mechanism to fetch the last commits for all open PRs, integrates that data into each PR object, updates the HTML generation to display those commits inline, and ensures the GitHub data cache is written out properly. Sequence diagram for fetching and displaying last commits for open PRs using GraphQLsequenceDiagram
participant User
participant scrumHelper
participant GitHubGraphQLAPI
User->>scrumHelper: Request PR data
scrumHelper->>GitHubGraphQLAPI: Batch GraphQL query for last commits of open PRs
GitHubGraphQLAPI-->>scrumHelper: Return commit data for each PR
scrumHelper->>scrumHelper: Attach commits to PR objects
scrumHelper->>User: Render PR list with last commits inline
Class diagram for PR object with attached last commitsclassDiagram
class PullRequest {
number: int
state: string
title: string
html_url: string
repository_url: string
_lastCommits: Commit[]
}
class Commit {
messageHeadline: string
committedDate: string
url: string
author: Author
}
class Author {
name: string
user: User
}
class User {
login: string
}
PullRequest "*" -- "*" Commit : has
Commit "1" -- "1" Author : by
Author "0..1" -- "1" User : may reference
Flow diagram for batch fetching and displaying PR commitsflowchart TD
A[Collect open PRs] --> B[Batch fetch last commits via GraphQL]
B --> C[Attach commits to PR objects]
C --> D[Generate HTML for PRs]
D --> E[Display PRs with last commits inline]
File-Level Changes
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 @vedansh-5 - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
@hpdang Please share your views. |
Thanks for your review, as mentioned this is PR is just a demonstration of the feature and isn't intended to be merged, it actually is better to show last 5 commits, if someone is working on a PR for days they would have a long list of commits, and we would only want to show the latest ones as previous ones would be there in previous reports, despite this thinking, this behavior can be changed to fetch all commits in a single fetch request for upto 50 open PRs. |
The image you have added prompts the message |
One known vulnerability is that, when users make a public fetch request(without github token) then the commits data is not fetched, and later on when they put github token and checks the last 5 commits checkbox for the same time the report is generated from cache(which does not generate commits data), this can be the issue you're facing, otherwise the feature works fine, and this vulnerability can be solved very easily, I noticed it just before I was going to bed so haven't worked on it. |
|
Thanks for your reviews @Preeti9764 this PR is just a demonstration that this feature can be achieved efficiently with the current caching mechanism, I do not want to refine this PR as I do not want it to be merged. |
I have not added the github token, and it was the image of that time, also i trying to say that if we did not use the github tokens this commit functionality will not work, as tokens are additional we cannot create commits adding just for tokens users.Thanks! |
I got the changes you have made here and but they limit the functionality of commit fetching , I have also thought of that but i did do that in my pr cause i don't want this kind of challenges for the users and want to keep things simpler for them and also enhanced the caching for same reasons. I known your concern of the code change as i addressed the issue you put personally , I will also do same here also. I have just implemented new changes so that we can have it , that can be refined according to your guidance @hpdang .Thanks for suggesting this! |
Signed-off-by: Vedansh Saini <[email protected]>
|
In the latest changes I have fixed this vulnerability, |
de3b202 to
9b1dfcb
Compare
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
|
@Preeti9764 @hpdang Please take a look whenever you can, thank you. |
|
|
|
|
Hey @Preeti9764 , this is the old bug as we knew it, since it has already been solved in the master and no conflicts arises for the feature branch this issue will be solved when this PR is merged. thank you |
Signed-off-by: Vedansh Saini <[email protected]>
|
@vedansh-5 I have checked it ,the report is correctly generated in the scrum preview but it sometimes not visible in google groups, gmail ..please look into this .Thanks! |
Could you please clarify as to when this behavior is seen under what usecase or parameters set, because I cannot replicate the issue. Thankyou. |
Signed-off-by: Vedansh Saini <[email protected]>
|
Please check if the issue persists, i added a dateBug handler. |
|
@Preeti9764 please test this |
|
I am experiencing some anonymous issues.Sometimes it fetches correctly but some time it shows like this , sometime in first refresh or maybe after 2,3 refersh , then have to refresh again . I starts showing just reviewed prs. 2025-07-02.22-22-46.mp4 |
|
@vedansh-5 @Preeti9764 I was thinking about this, is it possible to show commits only on the existing PRs and not on those made PRs? The reason people want to show commits because they haven't managed to close their existing PRs and want to make the progress visible. I don't think it is necessary to show commits on newly created/made PRs. What do you think about this idea? |
Yes makes sense, I will make the changes for this. |
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
|
Now the extension only show commits in the existing Prs and not in made Prs, Also I have fixed a race condition that was already fixed in master, but not in this feature branch. I have tested with all email clients with multiple refreshes, I did not encounter the problem. |
|
@hpdang @vedansh-5 can we have commits for draft prs too, for showing the progress in the draft PR. |
Good point, I had thought about that too and even worked on it for a bit, but decided it’s cleaner to handle draft PR commits in a separate PR. So I reverted that part here to keep this focused. let’s get this merged, and I’ll pick up the draft PR progress in a follow-up. thankyou. |
|
@vedansh-5 Please remove the unnecessary file scrumHelperdummy.js, which you have pushed .Thanks! |
I had added this file during a rebase, incase things went south, forgot to remove it. Thanks @Preeti9764
|
@Preeti9764 is it good now to merge? |













📌 Fixes
Fixes #51
📝 Summary of Changes
📸 Screenshots / Demo (if UI-related)
✅ Checklist
👀 Reviewer Notes
The core of this feature is the new
fetchCommitsForOpenPRsfunction which constructs and executes a single GraphQL query for all open PRs. The decision to make this a token-only feature is intentional. It acts as a "hook" to encourage users to add a token, which benefits both the user (better performance, higher rate limits) and the extension's stability by reducing unauthenticated API calls.This Pull Request serves as a technical demonstration to show that the "Show Commits" feature can be successfully implemented on top of our existing caching mechanism. The primary goal is to prove the concept without altering the current cache logic.
This PR is not intended to be merged. It is for review and discussion purposes only.
Summary by Sourcery
Use GraphQL to batch fetch the latest commits for open PRs, attach them to the PR data, and display commit details in the scrum summary to reduce REST API usage.
New Features:
Enhancements: