fix: preserve popup blocker text across report regeneration#538
Open
kaungmyatshwe1397 wants to merge 1 commit intofossasia:mainfrom
Open
fix: preserve popup blocker text across report regeneration#538kaungmyatshwe1397 wants to merge 1 commit intofossasia:mainfrom
kaungmyatshwe1397 wants to merge 1 commit intofossasia:mainfrom
Conversation
Contributor
Reviewer's GuideThis PR changes how the scrum report popup stores and restores the "blocker" text so that user-edited blocker content persists across report generations and cached loads, and is only reset on explicit data refresh, while also adding some small refactors/formatting tweaks. Sequence diagram for blocker text persistence on edit and regenerationsequenceDiagram
actor User
participant Popup as PopupScript_popup_js
participant DOM as PopupDOM
participant Storage as BrowserStorage
participant Helper as ScrumHelper_scrumHelper_js
User->>DOM: Edit blocker text in scrumReport
DOM-->>Popup: input / focusout event
Popup->>DOM: extractBlockerReasonFromReport(scrumReport)
Popup-->>Popup: normalizeBlockerReason(blockerReason)
Popup->>Storage: storage.local.set(userReason)
User->>Popup: Click Generate
Popup->>Storage: storage.local.get(platform)
Popup->>Helper: allIncluded(outputTarget)
Helper->>Storage: storage.local.get([... userReason ...])
Helper-->>Helper: userReason = storedUserReason || DEFAULT
Helper-->>Popup: Generated report HTML including blocker heading
Popup->>DOM: Render new scrumReport HTML
Popup->>DOM: applyBlockerReasonToReport(scrumReport, storedUserReason)
DOM-->>User: Updated report with preserved blocker text
Sequence diagram for restoring blocker text from cached scrum reportsequenceDiagram
participant Popup as PopupScript_popup_js
participant DOM as PopupDOM
participant Storage as BrowserStorage
Popup->>Storage: storage.local.get([... platformSpecificKeys ..., userReason])
Storage-->>Popup: lastScrumReportHtml, cacheKey, username, userReason
Popup-->>Popup: Compute expectedUsername and isUsernameMatch
Popup-->>Popup: Check ttlMs and matches
alt Cache_valid_and_matches_and_report_empty
Popup->>DOM: scrumReport.innerHTML = lastScrumReportHtml
Popup->>DOM: applyBlockerReasonToReport(scrumReport, userReason)
else Cache_expired_but_matches_and_report_empty
Popup->>DOM: scrumReport.innerHTML = lastScrumReportHtml
Popup->>DOM: applyBlockerReasonToReport(scrumReport, userReason)
end
Popup-->>DOM: Enable generate button if present
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
bot
added
javascript
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new blocker parsing logic in
popup.jsassumes the last<b>in the report is the blocker heading and that everything after it belongs to the blocker; this seems brittle and could break if more headings or sections are added later—consider anchoring on something more specific (e.g., known heading text or a dedicated wrapper) or at least limiting the range you clear/update. - In
applyBlockerReasonToReport, you remove all siblings after the heading and then append the new content at the end ofreportEl, which can both change section ordering and drop other trailing content; it may be safer to only replace the existing blocker content and insert the new span immediately after the heading. - The default blocker message string ('No Blocker at the moment') is now hard-coded in both
popup.jsandscrumHelper.js; extracting this into a shared constant or configuration would avoid divergence if the text ever changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new blocker parsing logic in `popup.js` assumes the last `<b>` in the report is the blocker heading and that everything after it belongs to the blocker; this seems brittle and could break if more headings or sections are added later—consider anchoring on something more specific (e.g., known heading text or a dedicated wrapper) or at least limiting the range you clear/update.
- In `applyBlockerReasonToReport`, you remove all siblings after the heading and then append the new content at the end of `reportEl`, which can both change section ordering and drop other trailing content; it may be safer to only replace the existing blocker content and insert the new span immediately after the heading.
- The default blocker message string ('No Blocker at the moment') is now hard-coded in both `popup.js` and `scrumHelper.js`; extracting this into a shared constant or configuration would avoid divergence if the text ever changes.
## Individual Comments
### Comment 1
<location path="src/scripts/scrumHelper.js" line_range="159-161" />
<code_context>
- userReason = 'No Blocker at the moment';
- browser.storage.local.remove(['userReason']);
+ // Keep user's blocking reason across Generate btn clicks
+ const storedUserReason = typeof items.userReason === 'string' ? items.userReason.trim() : '';
+ userReason = storedUserReason || 'No Blocker at the moment';
githubToken = items.githubToken;
gitlabToken = items.gitlabToken || '';
</code_context>
<issue_to_address>
**suggestion:** The default blocker message string is duplicated and could drift from `DEFAULT_BLOCKER_REASON` in `popup.js`.
This string duplicates `DEFAULT_BLOCKER_REASON` in `popup.js`. To avoid these drifting apart (e.g., if the text changes or gets localized), please source the default from a single shared constant or otherwise centralize it so both the popup and generated report stay in sync.
Suggested implementation:
```javascript
// Keep user's blocking reason across Generate btn clicks
const storedUserReason = typeof items.userReason === 'string' ? items.userReason.trim() : '';
const defaultBlockerReason = typeof window !== 'undefined' && typeof window.DEFAULT_BLOCKER_REASON === 'string'
? window.DEFAULT_BLOCKER_REASON
: 'No Blocker at the moment';
userReason = storedUserReason || defaultBlockerReason;
githubToken = items.githubToken;
```
If your build setup supports ES module imports between `popup.js` and `scrumHelper.js`, you may instead want to export `DEFAULT_BLOCKER_REASON` from `popup.js` and import it directly into `scrumHelper.js` to avoid relying on the global `window` object.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+159
to
+161
| // Keep user's blocking reason across Generate btn clicks | ||
| const storedUserReason = typeof items.userReason === 'string' ? items.userReason.trim() : ''; | ||
| userReason = storedUserReason || 'No Blocker at the moment'; |
Contributor
There was a problem hiding this comment.
suggestion: The default blocker message string is duplicated and could drift from DEFAULT_BLOCKER_REASON in popup.js.
This string duplicates DEFAULT_BLOCKER_REASON in popup.js. To avoid these drifting apart (e.g., if the text changes or gets localized), please source the default from a single shared constant or otherwise centralize it so both the popup and generated report stay in sync.
Suggested implementation:
// Keep user's blocking reason across Generate btn clicks
const storedUserReason = typeof items.userReason === 'string' ? items.userReason.trim() : '';
const defaultBlockerReason = typeof window !== 'undefined' && typeof window.DEFAULT_BLOCKER_REASON === 'string'
? window.DEFAULT_BLOCKER_REASON
: 'No Blocker at the moment';
userReason = storedUserReason || defaultBlockerReason;
githubToken = items.githubToken;If your build setup supports ES module imports between popup.js and scrumHelper.js, you may instead want to export DEFAULT_BLOCKER_REASON from popup.js and import it directly into scrumHelper.js to avoid relying on the global window object.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #537 by persisting the popup report’s “blocker” text (userReason) across repeated report generation and restoring it when cached report HTML is reloaded, only resetting it when the user explicitly refreshes data.
Changes:
- Stop clearing
userReasonon each report generation; instead default to a stored value when available. - Persist blocker edits made directly in the contenteditable report preview to
browser.storage.local. - When loading cached report HTML, re-apply the latest saved blocker text and clear it on “Refresh Data”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/scripts/scrumHelper.js | Uses stored userReason (trimmed) as the blocker text instead of resetting/removing it during generation. |
| src/scripts/popup.js | Adds blocker extraction/apply helpers, persists edits from the contenteditable report, restores blocker text on cached HTML load, and clears userReason on refresh. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+101
to
+108
| // Use last bold heading as the anchor to read or write only that part | ||
| function getBlockerHeading(reportEl) { | ||
| if (!reportEl) { | ||
| return null; | ||
| } | ||
|
|
||
| const headings = reportEl.querySelectorAll('b'); | ||
| return headings.length > 0 ? headings[headings.length - 1] : null; |
There was a problem hiding this comment.
getBlockerHeading() assumes the blocker section is always the last <b> in #scrumReport. Since #scrumReport is contenteditable, users can easily create additional <b> tags (e.g., by bolding any text), which will cause blocker extraction/apply to target the wrong anchor and can overwrite/delete unrelated report content. Consider anchoring the blocker section via a dedicated marker element/attribute generated by scrumHelper.js (e.g., a data-section="blocker" wrapper) or by matching the specific blocker heading text rather than relying on the last <b>.
Suggested change
| // Use last bold heading as the anchor to read or write only that part | |
| function getBlockerHeading(reportEl) { | |
| if (!reportEl) { | |
| return null; | |
| } | |
| const headings = reportEl.querySelectorAll('b'); | |
| return headings.length > 0 ? headings[headings.length - 1] : null; | |
| function isBlockerHeadingText(text) { | |
| return /^blockers?\b/i.test(typeof text === 'string' ? text.trim() : ''); | |
| } | |
| // Find the blocker heading by a stable marker first, then by heading text. | |
| function getBlockerHeading(reportEl) { | |
| if (!reportEl) { | |
| return null; | |
| } | |
| const blockerMarker = reportEl.querySelector('[data-section="blocker"]'); | |
| if (blockerMarker) { | |
| if (blockerMarker.matches('b')) { | |
| return blockerMarker; | |
| } | |
| const nestedHeading = blockerMarker.querySelector('b'); | |
| if (nestedHeading) { | |
| return nestedHeading; | |
| } | |
| } | |
| const headings = reportEl.querySelectorAll('b'); | |
| for (const heading of headings) { | |
| if (isBlockerHeadingText(heading.textContent)) { | |
| return heading; | |
| } | |
| } | |
| return null; |
Comment on lines
+111
to
+155
| function extractBlockerReasonFromReport(reportEl) { | ||
| const blockerHeading = getBlockerHeading(reportEl); | ||
| if (!blockerHeading) { | ||
| return null; | ||
| } | ||
|
|
||
| const parts = []; | ||
| let currentNode = blockerHeading.nextSibling; | ||
|
|
||
| while (currentNode) { | ||
| if (currentNode.nodeType === Node.TEXT_NODE) { | ||
| const text = currentNode.textContent.trim(); | ||
| if (text) { | ||
| parts.push(text); | ||
| } | ||
| } else if (currentNode.nodeName !== 'BR') { | ||
| const text = currentNode.textContent.trim(); | ||
| if (text) { | ||
| parts.push(text); | ||
| } | ||
| } | ||
|
|
||
| currentNode = currentNode.nextSibling; | ||
| } | ||
|
|
||
| return parts.join('\n').trim(); | ||
| } | ||
|
|
||
| function applyBlockerReasonToReport(reportEl, blockerReason) { | ||
| const blockerHeading = getBlockerHeading(reportEl); | ||
| if (!blockerHeading) { | ||
| return false; | ||
| } | ||
|
|
||
| let currentNode = blockerHeading.nextSibling; | ||
| while (currentNode) { | ||
| const nextNode = currentNode.nextSibling; | ||
| currentNode.remove(); | ||
| currentNode = nextNode; | ||
| } | ||
|
|
||
| reportEl.appendChild(document.createElement('br')); | ||
| const blockerSpan = document.createElement('span'); | ||
| blockerSpan.textContent = normalizeBlockerReason(blockerReason); | ||
| reportEl.appendChild(blockerSpan); |
There was a problem hiding this comment.
applyBlockerReasonToReport() removes all sibling nodes after the selected heading. Because the report is editable, users can add extra notes/content after the blocker section; this implementation will delete that content when cached HTML is loaded (and will also delete more than intended if the anchor heading is misidentified). Consider limiting the mutation to a specific blocker content container/span instead of removing everything after the heading.
Suggested change
| function extractBlockerReasonFromReport(reportEl) { | |
| const blockerHeading = getBlockerHeading(reportEl); | |
| if (!blockerHeading) { | |
| return null; | |
| } | |
| const parts = []; | |
| let currentNode = blockerHeading.nextSibling; | |
| while (currentNode) { | |
| if (currentNode.nodeType === Node.TEXT_NODE) { | |
| const text = currentNode.textContent.trim(); | |
| if (text) { | |
| parts.push(text); | |
| } | |
| } else if (currentNode.nodeName !== 'BR') { | |
| const text = currentNode.textContent.trim(); | |
| if (text) { | |
| parts.push(text); | |
| } | |
| } | |
| currentNode = currentNode.nextSibling; | |
| } | |
| return parts.join('\n').trim(); | |
| } | |
| function applyBlockerReasonToReport(reportEl, blockerReason) { | |
| const blockerHeading = getBlockerHeading(reportEl); | |
| if (!blockerHeading) { | |
| return false; | |
| } | |
| let currentNode = blockerHeading.nextSibling; | |
| while (currentNode) { | |
| const nextNode = currentNode.nextSibling; | |
| currentNode.remove(); | |
| currentNode = nextNode; | |
| } | |
| reportEl.appendChild(document.createElement('br')); | |
| const blockerSpan = document.createElement('span'); | |
| blockerSpan.textContent = normalizeBlockerReason(blockerReason); | |
| reportEl.appendChild(blockerSpan); | |
| function getBlockerReasonSpan(blockerHeading) { | |
| let currentNode = blockerHeading ? blockerHeading.nextSibling : null; | |
| let sawLineBreak = false; | |
| while (currentNode) { | |
| if (currentNode.nodeType === Node.TEXT_NODE) { | |
| if (currentNode.textContent.trim() === '') { | |
| currentNode = currentNode.nextSibling; | |
| continue; | |
| } | |
| return null; | |
| } | |
| if (currentNode.nodeName === 'BR') { | |
| sawLineBreak = true; | |
| currentNode = currentNode.nextSibling; | |
| continue; | |
| } | |
| if ( | |
| currentNode.nodeName === 'SPAN' && | |
| (currentNode.getAttribute('data-blocker-reason') === 'true' || sawLineBreak) | |
| ) { | |
| return currentNode; | |
| } | |
| return null; | |
| } | |
| return null; | |
| } | |
| function ensureBlockerReasonSpan(reportEl, blockerHeading) { | |
| let blockerSpan = getBlockerReasonSpan(blockerHeading); | |
| if (blockerSpan) { | |
| blockerSpan.setAttribute('data-blocker-reason', 'true'); | |
| return blockerSpan; | |
| } | |
| const br = document.createElement('br'); | |
| blockerSpan = document.createElement('span'); | |
| blockerSpan.setAttribute('data-blocker-reason', 'true'); | |
| if (blockerHeading.nextSibling) { | |
| reportEl.insertBefore(br, blockerHeading.nextSibling); | |
| } else { | |
| reportEl.appendChild(br); | |
| } | |
| if (br.nextSibling) { | |
| reportEl.insertBefore(blockerSpan, br.nextSibling); | |
| } else { | |
| reportEl.appendChild(blockerSpan); | |
| } | |
| return blockerSpan; | |
| } | |
| function extractBlockerReasonFromReport(reportEl) { | |
| const blockerHeading = getBlockerHeading(reportEl); | |
| if (!blockerHeading) { | |
| return null; | |
| } | |
| const blockerSpan = getBlockerReasonSpan(blockerHeading); | |
| if (!blockerSpan) { | |
| return null; | |
| } | |
| return blockerSpan.textContent.trim(); | |
| } | |
| function applyBlockerReasonToReport(reportEl, blockerReason) { | |
| const blockerHeading = getBlockerHeading(reportEl); | |
| if (!blockerHeading) { | |
| return false; | |
| } | |
| const blockerSpan = ensureBlockerReasonSpan(reportEl, blockerHeading); | |
| blockerSpan.textContent = normalizeBlockerReason(blockerReason); |
Comment on lines
+145
to
+153
| let currentNode = blockerHeading.nextSibling; | ||
| while (currentNode) { | ||
| const nextNode = currentNode.nextSibling; | ||
| currentNode.remove(); | ||
| currentNode = nextNode; | ||
| } | ||
|
|
||
| reportEl.appendChild(document.createElement('br')); | ||
| const blockerSpan = document.createElement('span'); |
There was a problem hiding this comment.
When restoring cached HTML, applyBlockerReasonToReport() replaces the original blocker markup (which scrumHelper.js renders via wrapCompactText() with an inline style) with a plain <span> that has no styling. This is a visible formatting regression for the blocker section. Consider reusing the existing blocker <span> (updating only its text) or applying the same inline style used in the generated report.
Suggested change
| let currentNode = blockerHeading.nextSibling; | |
| while (currentNode) { | |
| const nextNode = currentNode.nextSibling; | |
| currentNode.remove(); | |
| currentNode = nextNode; | |
| } | |
| reportEl.appendChild(document.createElement('br')); | |
| const blockerSpan = document.createElement('span'); | |
| let blockerSpan = null; | |
| let currentNode = blockerHeading.nextSibling; | |
| while (currentNode) { | |
| const nextNode = currentNode.nextSibling; | |
| if (!blockerSpan && currentNode.nodeType === Node.ELEMENT_NODE && currentNode.nodeName === 'SPAN') { | |
| blockerSpan = currentNode; | |
| } else { | |
| currentNode.remove(); | |
| } | |
| currentNode = nextNode; | |
| } | |
| reportEl.appendChild(document.createElement('br')); | |
| if (!blockerSpan) { | |
| blockerSpan = document.createElement('span'); | |
| } |
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
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 #537
📝 Summary of Changes
📸 Screenshots / Demo (if UI-related)
_ Testing demo of editing and replacing user custom blocker message and reset to initial state by refresh Data
Screen.Recording.2026-04-13.192140.mp4
_
✅ Checklist
👀 Reviewer Notes
Summary by Sourcery
Preserve and persist the custom blocker message across popup scrum report generations and cache refreshes.
Bug Fixes:
Enhancements: