Enhance GitHub Organization Selection in Setting #164
Conversation
Reviewer's GuideThis PR fixes the report preview bug by centralizing cache and state reset logic in scrumHelper.js (via resetReportState and forceGithubDataRefresh), integrating orgName into cache keys to ensure fresh data is fetched after an organization change, and updating popup.js to clear cached data, show a placeholder prompt, and reduce the org‐input debounce delay. Sequence diagram for organization change and report preview refreshsequenceDiagram
actor User
participant PopupJS as popup.js
participant Storage as chrome.storage.local
participant ScrumHelper as scrumHelper.js
User->>PopupJS: Input new organization name
PopupJS->>Storage: Set orgName and clear githubCache
PopupJS->>PopupJS: Show placeholder in report preview
User->>PopupJS: Click "Generate Report"
PopupJS->>ScrumHelper: generateScrumReport()
ScrumHelper->>Storage: Fetch orgName and githubCache
ScrumHelper->>ScrumHelper: resetReportState()
ScrumHelper->>ScrumHelper: Fetch fresh GitHub data
ScrumHelper->>PopupJS: Update report preview
Class diagram for updated cache and state management in scrumHelper.jsclassDiagram
class githubCache {
+data
+cacheKey
+timestamp
+ttl
+fetching
+queue
+subject
}
class scrumHelper {
+resetReportState(regenerateReport, outputTarget)
+forceGithubDataRefresh()
+fetchGithubData()
}
scrumHelper --> githubCache : 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:
- Consider extracting the hardcoded 2500ms debounce into a named constant so the timing is easier to adjust and understand.
- Move the inline HTML and style update for the “Organisation changed” message into a dedicated render function or CSS class rather than injecting raw markup.
- Refactor the manual reset of processed flags and subsequent calls in scrumHelper.js into a single reset method to reduce duplication and clarify the report regeneration flow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the hardcoded 2500ms debounce into a named constant so the timing is easier to adjust and understand.
- Move the inline HTML and style update for the “Organisation changed” message into a dedicated render function or CSS class rather than injecting raw markup.
- Refactor the manual reset of processed flags and subsequent calls in scrumHelper.js into a single reset method to reduce duplication and clarify the report regeneration flow.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hello @Preeti9764, I have reviewed your work and would like to make a suggestion for a better approach to this issue. |
@hpdang Please share your views on this. Thanks. |
|
What @vedansh-5 suggested sounds good to me |
The line you are referring to is already removed in changes of this pr, I will add the org name in cache, thanks! |
|
@hpdang i have made the required changes and the bug is corrected . Thanks! |
There was a problem hiding this comment.
Hey @Preeti9764 - I've reviewed your changes - here's some feedback:
- Move the
resetReportStateandforceGithubDataRefreshfunctions out of theallIncludedscope to avoid redeclaring them on every call and improve readability. - In popup.js you set
githubCacheto null in storage instead of removing it—usechrome.storage.local.remove('githubCache')to fully clear the cache. - You have repeated state-reset logic scattered in multiple fetch/processing branches—consolidate into a single reset call to reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Move the `resetReportState` and `forceGithubDataRefresh` functions out of the `allIncluded` scope to avoid redeclaring them on every call and improve readability.
- In popup.js you set `githubCache` to null in storage instead of removing it—use `chrome.storage.local.remove('githubCache')` to fully clear the cache.
- You have repeated state-reset logic scattered in multiple fetch/processing branches—consolidate into a single reset call to reduce duplication.
## Individual Comments
### Comment 1
<location> `src/scripts/popup.js:403` </location>
<code_context>
// Valid org: update storage and fetch data
- chrome.storage.local.set({ orgName: org }, function () {
- if (window.generateScrumReport) window.generateScrumReport();
+ chrome.storage.local.set({ orgName: org, githubCache: null }, function () {
+ const scrumReport = document.getElementById('scrumReport');
+ if (scrumReport) {
</code_context>
<issue_to_address>
Setting githubCache to null in chrome.storage.local may not clear all cached data.
Use chrome.storage.local.remove('githubCache') to ensure the cache is fully cleared and prevent ambiguity in cache handling.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Valid org: update storage and fetch data
chrome.storage.local.set({ orgName: org, githubCache: null }, function () {
const scrumReport = document.getElementById('scrumReport');
if (scrumReport) {
scrumReport.innerHTML = '<p style="text-align: center; color: #666; padding: 20px;">Organisation changed. Click "Generate Report" to fetch new data.</p>';
}
});
=======
// Valid org: update storage and fetch data
chrome.storage.local.remove('githubCache', function () {
chrome.storage.local.set({ orgName: org }, function () {
const scrumReport = document.getElementById('scrumReport');
if (scrumReport) {
scrumReport.innerHTML = '<p style="text-align: center; color: #666; padding: 20px;">Organisation changed. Click "Generate Report" to fetch new data.</p>';
}
});
});
>>>>>>> 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/popup.js
Outdated
| // Valid org: update storage and fetch data | ||
| chrome.storage.local.set({ orgName: org }, function () { | ||
| if (window.generateScrumReport) window.generateScrumReport(); | ||
| chrome.storage.local.set({ orgName: org, githubCache: null }, function () { | ||
| const scrumReport = document.getElementById('scrumReport'); | ||
| if (scrumReport) { | ||
| scrumReport.innerHTML = '<p style="text-align: center; color: #666; padding: 20px;">Organisation changed. Click "Generate Report" to fetch new data.</p>'; | ||
| } | ||
| }); |
There was a problem hiding this comment.
suggestion: Setting githubCache to null in chrome.storage.local may not clear all cached data.
Use chrome.storage.local.remove('githubCache') to ensure the cache is fully cleared and prevent ambiguity in cache handling.
| // Valid org: update storage and fetch data | |
| chrome.storage.local.set({ orgName: org }, function () { | |
| if (window.generateScrumReport) window.generateScrumReport(); | |
| chrome.storage.local.set({ orgName: org, githubCache: null }, function () { | |
| const scrumReport = document.getElementById('scrumReport'); | |
| if (scrumReport) { | |
| scrumReport.innerHTML = '<p style="text-align: center; color: #666; padding: 20px;">Organisation changed. Click "Generate Report" to fetch new data.</p>'; | |
| } | |
| }); | |
| // Valid org: update storage and fetch data | |
| chrome.storage.local.remove('githubCache', function () { | |
| chrome.storage.local.set({ orgName: org }, function () { | |
| const scrumReport = document.getElementById('scrumReport'); | |
| if (scrumReport) { | |
| scrumReport.innerHTML = '<p style="text-align: center; color: #666; padding: 20px;">Organisation changed. Click "Generate Report" to fetch new data.</p>'; | |
| } | |
| }); | |
| }); |
src/scripts/scrumHelper.js
Outdated
| async function forceGithubDataRefresh() { | ||
| log('Force refreshing GitHub data'); | ||
|
|
||
| // Clear cache from storage | ||
| await new Promise(resolve => { | ||
| chrome.storage.local.remove('githubCache', resolve); | ||
| }); | ||
|
|
||
| // Reset report state | ||
| resetReportState(false); | ||
|
|
||
| // Fetch fresh data | ||
| try { | ||
| await fetchGithubData(); | ||
| return { success: true, message: 'Data refreshed successfully' }; | ||
| } catch (error) { | ||
| logError('Force refresh failed:', error); | ||
| return { success: false, error: error.message }; | ||
| } | ||
| } |
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
| function handleRefresh() { | ||
| hasInjectedContent = false; // Reset the flag before refresh | ||
| resetReportState(false, 'email'); | ||
| allIncluded(); | ||
| } |
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.
There was a problem hiding this comment.
There is no need of multiple entries to be stored in the extension. For our projects' scope single stored entries are more than enough and serves us efficiently.
Also its a major change in the caching mechanism, if you feel they are necessary we can open another ticket for that, please don't push several things in a single PR.
Thanks!
src/scripts/scrumHelper.js
Outdated
|
|
||
| // Global cache object | ||
| let githubCache = { | ||
| data: null, | ||
| cacheKey: null, | ||
| timestamp: null, | ||
| ttl: null, | ||
| fetching: false, | ||
| queue: [], | ||
| subject: null | ||
| }; | ||
|
|
There was a problem hiding this comment.
Please revert these changes!
There was a problem hiding this comment.
thanks for pointing i will remove ttl, fetching, and queue as they are not currently used .
src/scripts/scrumHelper.js
Outdated
| /** | ||
| * Resets all report processing flags and state, then optionally regenerates the report | ||
| * @param {boolean} regenerateReport - Whether to regenerate the report after reset | ||
| * @param {string} outputTarget - The output target ('popup' or 'email') | ||
| */ | ||
| function resetReportState(regenerateReport = false, outputTarget = 'popup') { | ||
| log('Resetting report state'); | ||
|
|
||
| // Reset all processing flags | ||
| issuesDataProcessed = false; | ||
| prsReviewDataProcessed = false; | ||
| hasInjectedContent = false; | ||
|
|
||
| // Reset data arrays | ||
| lastWeekArray = []; | ||
| nextWeekArray = []; | ||
| reviewedPrsArray = []; | ||
| githubPrsReviewDataProcessed = {}; | ||
|
|
||
| // Clear cached data | ||
| githubCache.data = null; | ||
| githubCache.cacheKey = null; | ||
| githubCache.timestamp = null; | ||
| githubCache.subject = null; | ||
|
|
||
| log('Report state reset complete'); | ||
|
|
||
| // Regenerate report if requested | ||
| if (regenerateReport) { | ||
| log('Regenerating report after reset'); | ||
| if (outputTarget === 'popup') { | ||
| writeGithubIssuesPrs(); | ||
| writeGithubPrsReviews(); | ||
| } else { | ||
| // For email context, trigger a fresh data fetch | ||
| fetchGithubData(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Forces a refresh of GitHub data by clearing cache and fetching new data | ||
| * @returns {Promise} Promise that resolves when refresh is complete | ||
| */ | ||
| async function forceGithubDataRefresh() { | ||
| log('Force refreshing GitHub data'); | ||
|
|
||
| // Clear cache from storage | ||
| await new Promise(resolve => { | ||
| chrome.storage.local.remove('githubCache', resolve); | ||
| }); | ||
|
|
||
| // Reset report state | ||
| resetReportState(false); | ||
|
|
||
| // Fetch fresh data | ||
| try { | ||
| await fetchGithubData(); | ||
| return { success: true, message: 'Data refreshed successfully' }; | ||
| } catch (error) { | ||
| logError('Force refresh failed:', error); | ||
| return { success: false, error: error.message }; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Just a small request - let's avoid pushing AI-generated code directly. It often misses context and might not align well with the project's structure.. Thanks!
There was a problem hiding this comment.
if you don' t have the context let me add it for you , it is a suggestion from sourcery-ai to have refactor the manual reset of processed flags and subsequent calls in scrumHelper.js into a single reset method to reduce duplication and clarify the report regeneration flow and i find it benifical because it reduces duplication ,improves clarity and the function is only used in places where a full reset is appropriate, and the code is now easier to maintain and less error-prone. Thanks!
@vedansh-5 The only cache object used is githubCache, which is a single object in memory,there is no array of caches, no logic for storing multiple cache objects, and no code that would result in multiple cache entries being kept in storage and queue and fetching fields are only used for managing concurrent fetches . Thanks! |
|
@vedansh-5 The reset logic is used in multiple places—refresh actions, after fetches, after processing data, and more. Centralizing it in resetReportState() keeps the code DRY (Don’t Repeat Yourself), reliable, and easy to maintain.I hope you get the logic for new function addition change. Thanks! |
|
Hello Preeti, I will soon make a review on this, I am facing downtime on
github.com, this might be because of rate limiter. I'll make a reviews
asap. Thanks!
(Ps: making this reply from gmail reply.)
…On Thu, Jun 26, 2025, 3:40 PM Preeti Yadav ***@***.***> wrote:
*Preeti9764* left a comment (fossasia/scrum_helper#164)
<#164 (comment)>
@vedansh-5 <https://github.com/vedansh-5> The reset logic is used in
multiple places—refresh actions, after fetches, after processing data, and
more. Centralizing it in resetReportState() keeps the code DRY (Don’t
Repeat Yourself), reliable, and easy to maintain.I hope you get the logic
for new function addition change. Thanks!
—
Reply to this email directly, view it on GitHub
<#164 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASRZUKRFGE4RCAGNYTA4JUL3FPBI5AVCNFSM6AAAAAB77AZ2ECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMBXHE2DQMZZGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
|
I’m not against the logic or the improvement itself. Just to clarify, my concern is that the code - as you mentioned- was directly suggested by Sorcery AI and included as-is, with its comments and structure intact. We generally avoid adding AI-generated code directly to the codebase. If you're using an AI suggestion, it’s best to adapt and rewrite it in your own style so it fits better with the rest of the code. Also, for this reason, please remove the JSDocs - they’re not needed here. Thanks! |
|
About the cache code - sorry for the confusion earlier. My comment was based on how it appeared in GitHub's UI, it was being shown as a new insertion, I misunderstood it as a duplicate. That was a mistake on my part, I now realize that the code was already part of the base and it's actually needed. TTL, Fetching and Queues are being used in many parts of the code. Could you please revert that removal? Thanks! |
Yeah will raise a issue for this , currently this bug needs to be fixed . Thanks! |
The JSDocs you’re referring to are comments that help explain what a function does, its parameters, and its return values. They can be very helpful for new contributors to quickly understand the purpose and usage of a function without having to read through all the code. if you want i can change the format I’ll write it in our own style rather than using generic JSDoc blocks.Thanks! |
I'm aware that JSDocs are meant to help explain functions, but in this case the function name is already self-explanatory. Also, since both the function and the JSDocs were generated by AI, I imply they can be removed. I leave it up to you. |
Thanks for your suggestions , as i said the reset logic is used in multiple places—refresh actions, after fetches,after processing data and others having a centralised function for it would be helpful in further development and easy to use which is my objective behind adding this function and I am open to change . |
organizationReview.mp4Another thing is when I clear the org name it still fetches the data for the org. In the current implementation we have to click the set button after clearing the name. This behavior is confusing, ideally if we have cleared the org name, either the textbox should remove the org in accordance to your new PR #168 or set it to fossasia as default without clicking the set button. Thanks! |
https://github.com/user-attachments/assets/355652f0-5b08-4852-b399-254499d39045 |
That behaviour will be when we will merge that pr , for now having it without organization name or org name empty is not a feature , that will be added in the pr#168.Thanks! |
|
Please move the set button little to the right and make the org input bar a bit longer in accordance with the rest of the UI |
I was suggesting the first part to reduce some of your work as when that PR is merged this would be implemented, but till then for this PR we must go for the second option
This addition is also a necessity for this problem: orgRev4.mp4 |
|
To put all this in a single message: |
|
I have made the required changes , now the report is generated correctly , and currently user need to input there org name and click the set button, it gets validated and they can move forward for using the extension, as of now leaving org name empty has no output , but in PR#168 , i will add this line in bubble of org name that " leaving org name empty will fetch your all github actvities " so user can go for that , for now if you set the org name empty it fetches the data of fossasia . |
revOrg5.mp4I still have to deliberately press the set button even for clearing the org. Also the organization name is not set after clearing the org until we reload the extension.
@Preeti9764 Please implement this behavior. Tagging @hpdang for her views on this
I agree with this behavior, it will work well when the concerned PR is merged. |
|
I have made the ui changes . |
|
@hpdang @vedansh-5 The current behaviour sets the default to fossasia unless they change to something else. If the user erases the Field to change it but changes their mind they would expect because they just cleared out the field without clicking on something solid like Set button , It should keep it default to their previous input ... Changing it to fossasia without proper user decision will confuse the user and reduce the validity of the Set button.On the other hand the organisation should be set to Fossasia by default until user gives personal input as well as if the field is empty and they click on Set specifically.Thanks! |
There was a problem hiding this comment.
Looks good, although, the behavior of set button as described by you in your last comment, is not advisable, let's look at it this way, if someone clears the orgname its obvious they do not want to fetch data of that org now. And in the current implementation they will see the data of the previously set org only, this behavior will create confusion, since in the input boxs' placeholder it is written that default org is fossasia.
Please change this behavior to set the org to fossasia when the input is cleared.
Apart from that the changes looks good to me. Thankyou.
|
@hpdang @vedansh-5 as someone changes their org name to another org than fossasia, that get stored into Chrome storage and if user erase the input of org and did not click the set button their default which they have chosen previously will work , not fossasia cause fossasia is default untill you have not changed your org input or set to other, if user again wants the activities for fossasia , they will set it back to fossasia , which would ideal flow look like .I have removed the default from the placeholder. If someone clears the org input and didn't change it ,fetching fossasia activities can create confusion . Thanks!
|
hpdang
changed the title
|
@hpdang @vedansh-5 made the changes .Thanks! |
|
Tested and worked for me. @Preeti9764 please resolve conflict. @vedansh-5 did you test this too? |
|
Just tested this, it works fine with all clients, orgname behavior is as expected.
|
|
@hpdang Resolved the confilicts, ready to merge .Thanks! |
📌 Fixes
Fixes #163
📝 Summary of Changes
-The preview report will be generated when button get clicked after the organization name change.
📸 Screenshots / Demo (if UI-related)
✅ Checklist
👀 Reviewer Notes
Add any special notes for the reviewer here
Summary by Sourcery
Fix report preview bug by clearing cache and updating UI placeholder on organization change, adjusting debounce timing, and forcing preview content to refresh
Bug Fixes:
Enhancements:
Summary by Sourcery
Fix report preview not updating correctly after changing the organization by resetting cache and state, adjusting debounce timing, and forcing data refresh.
Bug Fixes:
Enhancements: