Enhancement : improve UX for scrum report editing #558
Open
kaungmyatshwe1397 wants to merge 6 commits intofossasia:mainfrom
Open
Enhancement : improve UX for scrum report editing #558kaungmyatshwe1397 wants to merge 6 commits intofossasia:mainfrom
kaungmyatshwe1397 wants to merge 6 commits intofossasia:mainfrom
Conversation
…ble to edit the body only while heading remain locked.
Contributor
Reviewer's GuideImplements structured scrum report sections with locked headings and editable bodies in the popup, adds robust HTML export/sanitization for copy/email actions (with legacy fallback), and wires popup lifecycle and storage logic to correctly toggle editability and regenerate reports when needed. Sequence diagram for scrum report export (copy and insert in email)sequenceDiagram
actor popupUser
participant Popup
participant buildExportableScrumHtml
participant normalizeLegacyReportHtml
participant EmailClientAdapter
participant Clipboard
popupUser->>Popup: Click insertInEmail
Popup->>buildExportableScrumHtml: buildExportableScrumHtml(scrumReport)
buildExportableScrumHtml->>buildExportableScrumHtml: Find [data-scrum-report]
alt Structured_scrum_report
buildExportableScrumHtml->>buildExportableScrumHtml: Collect [data-scrum-section]
buildExportableScrumHtml->>buildExportableScrumHtml: For_each_section
buildExportableScrumHtml->>buildExportableScrumHtml: normalizeSectionBodyHtml(bodyEl)
buildExportableScrumHtml->>buildExportableScrumHtml: escapeHtmlForExport(headingText)
buildExportableScrumHtml-->>Popup: Joined_clean_HTML
else Legacy_html
buildExportableScrumHtml->>normalizeLegacyReportHtml: normalizeLegacyReportHtml(innerHTML)
normalizeLegacyReportHtml->>normalizeLegacyReportHtml: stripEditAttributes(wrapper)
normalizeLegacyReportHtml-->>buildExportableScrumHtml: Normalized_HTML
buildExportableScrumHtml-->>Popup: Normalized_HTML
end
Popup->>EmailClientAdapter: injectContent(body, exportHtml, contentChangeEventType)
popupUser->>Popup: Click copyReport
Popup->>buildExportableScrumHtml: buildExportableScrumHtml(scrumReport)
buildExportableScrumHtml-->>Popup: Exportable_HTML
Popup->>Clipboard: Copy_via_temporary_hidden_div
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 6 security issues, 1 other issue, and left some high level feedback:
Security issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
wrapper.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
tempDiv.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
scrumReport.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- The structured scrum report HTML is currently built with large template strings in
writeScrumBody; consider constructing the DOM nodes programmatically (or at least centralizing the section/attribute names) to reduce the risk of markup/escaping bugs and keep the structure easier to evolve. isStructuredScrumReportHtmlrelies on a rawhtml.includes('data-scrum-report="true"')check against cached HTML, which is brittle to formatting/attribute ordering; parsing into a temporary element and reusing the same selector logic assyncScrumReportEditModewould make the detection more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The structured scrum report HTML is currently built with large template strings in `writeScrumBody`; consider constructing the DOM nodes programmatically (or at least centralizing the section/attribute names) to reduce the risk of markup/escaping bugs and keep the structure easier to evolve.
- `isStructuredScrumReportHtml` relies on a raw `html.includes('data-scrum-report="true"')` check against cached HTML, which is brittle to formatting/attribute ordering; parsing into a temporary element and reusing the same selector logic as `syncScrumReportEditMode` would make the detection more robust.
## Individual Comments
### Comment 1
<location path="src/scripts/scrumHelper.js" line_range="1189" />
<code_context>
+ const blockerText = buildBlockerTextHtml();
+ const { heading1, heading2, heading3 } = buildScrumSectionHeadings();
+
+ // sturcture sections with locked headings and editable bodies
+ const popupContent = `<div data-scrum-report="true">
+<div data-scrum-section="1">
+<div data-scrum-heading="1" contenteditable="false"><b>${heading1}</b></div>
</code_context>
<issue_to_address>
**nitpick (typo):** Fix typo in the comment describing structured sections.
The comment currently says `sturcture sections`—please correct to `structure sections` for clarity.
```suggestion
// structure sections with locked headings and editable bodies
```
</issue_to_address>
### Comment 2
<location path="src/scripts/popup.js" line_range="472" />
<code_context>
wrapper.innerHTML = html;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 3
<location path="src/scripts/popup.js" line_range="472" />
<code_context>
wrapper.innerHTML = html;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `wrapper.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="src/scripts/popup.js" line_range="855" />
<code_context>
tempDiv.innerHTML = buildExportableScrumHtml(scrumReport);
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 5
<location path="src/scripts/popup.js" line_range="855" />
<code_context>
tempDiv.innerHTML = buildExportableScrumHtml(scrumReport);
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `tempDiv.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 6
<location path="src/scripts/scrumHelper.js" line_range="1216" />
<code_context>
scrumReport.innerHTML = popupContent;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 7
<location path="src/scripts/scrumHelper.js" line_range="1216" />
<code_context>
scrumReport.innerHTML = popupContent;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `scrumReport.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const blockerText = buildBlockerTextHtml(); | ||
| const { heading1, heading2, heading3 } = buildScrumSectionHeadings(); | ||
|
|
||
| // sturcture sections with locked headings and editable bodies |
Contributor
There was a problem hiding this comment.
nitpick (typo): Fix typo in the comment describing structured sections.
The comment currently says sturcture sections—please correct to structure sections for clarity.
Suggested change
| // sturcture sections with locked headings and editable bodies | |
| // structure sections with locked headings and editable bodies |
| if (!html || typeof html !== 'string') return ''; | ||
|
|
||
| const wrapper = document.createElement('div'); | ||
| wrapper.innerHTML = html; |
Contributor
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| if (!html || typeof html !== 'string') return ''; | ||
|
|
||
| const wrapper = document.createElement('div'); | ||
| wrapper.innerHTML = html; |
Contributor
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a wrapper.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| const scrumReport = document.getElementById('scrumReport'); | ||
| const tempDiv = document.createElement('div'); | ||
| tempDiv.innerHTML = scrumReport.innerHTML; | ||
| tempDiv.innerHTML = buildExportableScrumHtml(scrumReport); |
Contributor
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| const scrumReport = document.getElementById('scrumReport'); | ||
| const tempDiv = document.createElement('div'); | ||
| tempDiv.innerHTML = scrumReport.innerHTML; | ||
| tempDiv.innerHTML = buildExportableScrumHtml(scrumReport); |
Contributor
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a tempDiv.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| if (scrumReport) { | ||
| log('Found popup div, updating content'); | ||
| scrumReport.innerHTML = content; | ||
| scrumReport.innerHTML = popupContent; |
Contributor
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| if (scrumReport) { | ||
| log('Found popup div, updating content'); | ||
| scrumReport.innerHTML = content; | ||
| scrumReport.innerHTML = popupContent; |
Contributor
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a scrumReport.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the scrum report editing UX in the extension popup by introducing a structured report layout (locked headings + editable bodies) and ensuring copy/email exports generate clean HTML rather than popup-specific markup.
Changes:
- Generate scrum reports as structured sections in the popup (non-editable headings, editable bodies).
- Update popup behavior to detect structured reports, toggle editability appropriately, and export sanitized/clean HTML for Copy / Insert in Email.
- Add fallback export handling for legacy (non-structured) scrum report HTML.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/scripts/scrumHelper.js |
Generates structured popup HTML with locked headings and separate editable bodies; keeps email output as clean non-structured HTML. |
src/scripts/popup.js |
Detects structured scrum report markup, syncs popup edit mode, and builds export-ready HTML (Copy / Insert in Email) with legacy fallback. |
src/popup.html |
Sets the scrum report container default to non-editable to support structured content. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
632
to
+636
| if (reportEmpty && lastScrumReportHtml && matches) { | ||
| if (!isStructuredScrumReportHtml(lastScrumReportHtml)) { | ||
| setGenerateButtonLoading(generateBtn, true); | ||
| window.generateScrumReport(); | ||
| return; |
There was a problem hiding this comment.
bootstrapScrumReportOnPopupLoad forces a full regeneration when cached lastScrumReportHtml is legacy (non-structured), even though the PR description mentions preserving compatibility via a fallback export path. This behavior can discard a user's previously edited cached report within the TTL window. Consider rendering the legacy cached HTML as-is (and rely on buildExportableScrumHtml() for clean copy/email), or explicitly migrating legacy HTML to the structured format instead of regenerating unconditionally.
| const blockerText = buildBlockerTextHtml(); | ||
| const { heading1, heading2, heading3 } = buildScrumSectionHeadings(); | ||
|
|
||
| // sturcture sections with locked headings and editable bodies |
There was a problem hiding this comment.
Typo in the comment: "sturcture" should be "structure".
Suggested change
| // sturcture sections with locked headings and editable bodies | |
| // structure sections with locked headings and editable bodies |
| return clone.innerHTML || ''; | ||
| } | ||
|
|
||
| // Change clean html from structure scrumReport before sending email |
There was a problem hiding this comment.
Wording/grammar in this comment is unclear ("Change clean html from structure scrumReport..."). Consider rephrasing to something like "Build clean HTML from structured scrum report content before exporting" to avoid confusion for future maintainers.
Suggested change
| // Change clean html from structure scrumReport before sending email | |
| // Build clean HTML from structured scrum report content before sending email. |
Comment on lines
1165
to
1166
| const weekOrDay = yesterdayContribution ? 'yesterday' : 'the period'; | ||
| const weekOrDay2 = 'today'; |
There was a problem hiding this comment.
In buildScrumSectionHeadings, weekOrDay is only used in the yesterdayContribution branch; when yesterdayContribution is false it becomes an unused variable. Consider moving the declaration inside the if block or removing it to keep the function minimal.
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 #557
📝 Summary of Changes
CopyandInsert in Emailgenerate clean HTML instead of popup-specific markup📸 Screenshots / Demo (if UI-related)
Editable report body with fixed headings
Screen.Recording.2026-04-16.101338.mp4
Testing the update report to check sanitizing when report insert into email
Screen.Recording.2026-04-16.101622.mp4
✅ Checklist
👀 Reviewer Notes
This PR is intended as a focused UX enhancement for scrum report editing rather than a broad refactor.
Summary by Sourcery
Improve the scrum report editing UX by structuring generated reports into locked-heading sections while ensuring clean HTML export for email and copy actions.
New Features:
Bug Fixes:
Enhancements: