Added commits #136
Added commits #136
Conversation
Reviewer's GuideThis PR extends the Scrum report generator by fetching a user’s commits for open PRs within the selected date range, integrating them into the “What did I do” section, and refactors the data-fetching/processing flow for clarity. It also adds a loading spinner to the Generate Report button and tidies up formatting and indentation. Sequence diagram for generating Scrum report with commit fetchingsequenceDiagram
actor User
participant UI as Popup/Email UI
participant Button as Generate Report Button
participant ScrumHelper as scrumHelper.js
participant GitHub as GitHub API
User->>Button: Click 'Generate Report'
Button->>ScrumHelper: Trigger allIncluded('popup')
ScrumHelper->>ScrumHelper: fetchGithubData()
ScrumHelper->>GitHub: Fetch issues, PRs, user data
GitHub-->>ScrumHelper: Return issues, PRs, user data
ScrumHelper->>GitHub: For each open PR, fetch commits
GitHub-->>ScrumHelper: Return commits for each PR
ScrumHelper->>ScrumHelper: Integrate commits into report data
ScrumHelper->>UI: Update Scrum report content
ScrumHelper->>Button: Update button state (spinner/refresh)
UI-->>User: Show updated Scrum report with commits
Flow diagram for Scrum report data processing with commit integrationflowchart TD
A[User clicks Generate Report] --> B[fetchGithubData]
B --> C[fetchGithubIssues]
C --> D[fetchCommitsForOpenPRs]
D --> E[processAllData]
E --> F[processGithubData]
F --> G[processPrReviews]
F --> H[Integrate commits into report]
G --> I[writeScrumBody]
H --> I
I --> J[Update UI with Scrum report]
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 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.
|
@Preeti9764 the commits were not shown for me when I tested. Could you please share a screenshot compared the two generated scrum (before added commits) and (after added commits). Thanks! |
Sure, I will add Screenshots. Actually, this pr helps in fetching the commits we made in our existing PRs which are open and which we are working on, to get track of our commit activities in the selected period. Thanks! |
|
@hpdang @vedansh-5, please check the changes i have made once . Thanks! |
|
In your branch fetch calls are being made everytime I generate scrum content, that shouldn't be the case, fetched data should be saved and retrieved from browser local storage as its being saved in master branch. Also the Thanks! |
|
@vedansh-5, I have fixed the bugs you mentioned and implemented a more robust caching mechanism, which should work correctly. Can you please review it once? Thanks! |
|
@hpdang Can you please check once are commits fetched for you correctly ?Thanks! |
There was a problem hiding this comment.
@Preeti9764 Thank you for your hardwork and teh improvements in this PR. After reviewing the changes, I wanted to share some feedback and suggestions for further improvement.
-
Increased Code Complexity:
The new multi-entry caching mechanism introduces a lot of additional logic for managing separate cache entries for each username and date range. Since our extension is designed for users to generate one report at a time, this complexity doesn’t provide a tangible benefit and makes the codebase harder to maintain and debug. -
Risk of Cache Bloat:
By creating a new cache entry for every unique combination of username and date range, the cache can grow without bounds over time. This could eventually lead to excessive storage usage, which may impact performance or even hit browser storage limits. -
User Experience and UI Consistency:
The UI presents a single cache TTL setting and a single “refresh cache” button, which suggests to users that there is only one cache to manage. The multi-entry cache model can make the cache’s behavior less predictable and potentially confusing for users, as refreshing or changing settings may not always have the expected effect. -
Error Handling and Data Validation:
There is limited error handling for cases where cache entries might be malformed or incomplete. This could lead to subtle bugs or failures in data retrieval. -
Missed Opportunity for Batch Queries:
Earlier, I had suggested using batch queries to optimize our API usage and avoid hitting the GitHub requests-per-hour-per-IP limit. However, this approach hasn’t been implemented in the current PR. Leveraging GitHub GraphQL batch queries would help us stay within rate limits and improve the extension’s efficiency, especially for users with a lot of activity. -
Preferred Cache Removal Workflow:
One important point is that in the old caching mechanism, the cache was being removed whenever the cache time was changed, ensuring that only the relevant data for the current time period was stored. In the new mechanism, even when new data is fetched, previous data for other periods is still stored. The earlier approach is our preferred workflow, as the extension only fetches the report for one time period at an instance, and this keeps the cache clean and relevant.
I have raised another PR #147 to demonstrate how the commits-fetching function can be implemented using the GitHub GraphQL API and batch queries, while retaining the old caching mechanism. I do not want my PR to be merged, it is just to show how this can be achieved efficiently with the current caching approach.
Given these points, I kindly request that we revert to the older, simpler caching mechanism, which is more aligned with our current workflow and UI.
@hpdang @mariobehling please share your views on this approach.
| const githubCache = { | ||
| caches: {}, // Store multiple cache entries by key | ||
| fetching: false, | ||
| queue: [], |
There was a problem hiding this comment.
Multiple cache entries, not really needed, bloats the extension storage.
| throw new Error('GitHub API rate limit exceeded. Please try again in a few minutes.'); | ||
| } | ||
| if (!issuesRes.ok || !prRes.ok || !userRes.ok || !allOpenPrsRes.ok) { | ||
| throw new Error(`Failed to fetch data from GitHub. Status: ${issuesRes.status}, ${prRes.status}, ${userRes.status}, ${allOpenPrsRes.status}`); |
There was a problem hiding this comment.
These messages can't be closed by clicking the cross icon, also they do not close by themselves after some time.
| githubCache.fetching = true; | ||
|
|
||
| try { | ||
| let issueUrl = `https://api.github.com/search/issues?q=author%3A${username}+org%3Afossasia+created%3A${startingDate}..${endingDate}&per_page=100`; |
There was a problem hiding this comment.
I noticed a missing entry that should've been here.
Your branch:
Master branch:
| if (editor && editor.body) { | ||
| log('Injecting scrum into email body.'); | ||
| // Use the proper injection method instead of directly setting innerHTML | ||
| const success = window.emailClientAdapter.injectContent( |
There was a problem hiding this comment.
Another bug I observed is I can not change the pointer to another message body ( I checked only for google groups).
bodyBugPreeti.mp4
Fixes #51
📝 Summary of Changes
The Commits made in existing PRs will be visible in generated Scrum along with the base PR.
📸 Screenshots / Demo (if UI-related)
Summary by Sourcery
Add support for retrieving commits from open pull requests and integrate them into the Scrum report, while refactoring the GitHub data pipeline and caching logic, and enhancing UI feedback during report generation.
New Features:
Bug Fixes:
Enhancements: