fixed report preview bug #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>'; | |
| } | |
| }); | |
| }); |
| 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.| 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!
|
|
||
| // 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 . |
|
To clarify, my comment wasn’t about the function logic itself. I was specifically referring to the JSDocs that were included verbatim from the AI-generated suggestion. I’m suggesting they be written manually, in a clearer and more tailored way, so they’re actually helpful for new contributors. That would make a lot more sense and maintain consistency. |
if you mean that , i mentioned i will change the format for comments ,also as they are JSDoc and are easily understandable by developers.Thanks! |
You edited the concerning message 23 minutes ago (after my comment was made). 🙂
|
|
@hpdang I have made suggested changes , this fixes current bug and good to be merged from my end Thanks! |
|
@Preeti9764 please also resolve the conflicts |
|
@hpdang I have resolved the conflicts.Thanks |
|
@vedansh-5 please review |
There was a problem hiding this comment.
Left a comment, apart from that the PR looks good to me.
Also, I observed the home button was gone, it was completely white(could not be seen), but was clickable.
But upon refreshing this bug was resolved, I'm not sure if it was something concerned to this PR or not, did you happen to notice the same too @Preeti9764 @hpdang ?
| @@ -422,7 +427,7 @@ document.addEventListener('DOMContentLoaded', function () { | |||
| if (toastDiv.parentNode) toastDiv.parentNode.removeChild(toastDiv); | |||
| }, 3000); | |||
| }); | |||
| }, 3000); | |||
| }, 2500); | |||
There was a problem hiding this comment.
When the organization name is changed, the content takes a noticeable amount of time to get removed. This delay can lead to multiple points of confusion for users:
- the delay might be mistaken as the extension hanging or lagging, giving an impression of poor performance or unresponsiveness.
- the mismatch between user input and visual feedback breaks consistency in the experience, especially for users who expect immediate UI changes.
- overtime this delay can reduce user trust in the accuracy or responsiveness of the extension.
This might be due to an interval/polling mechanism that delays cleanup. This time period can/should be reduced.
lagging.preeti.mp4
📌 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: