Closed
Conversation
Contributor
Reviewer's GuideImplements front-end validation and UX safeguards around report generation by disabling the Generate Report button until username and date inputs are valid, wiring a dedicated generateReport handler that checks for a stored GitHub token before fetching data, and updating the popup markup to support the new behavior and output container. Sequence diagram for generateReport flow with token check and validationsequenceDiagram
actor User
participant Popup as PopupUI
participant Script as popup_js
participant Storage as chrome_storage_local
participant GitHub as GitHub_API
User->>Popup: Open_popup
Popup->>Script: DOMContentLoaded
Script->>Popup: applyI18n
Script->>Popup: Set_generateReport_disabled_true
Script->>Popup: Attach_input_listeners
User->>Popup: Type_username
Popup->>Script: username_input_event
Script->>Script: validateForm
Script->>Popup: Enable_or_disable_generateReport
User->>Popup: Select_from_and_to_dates
Popup->>Script: date_change_events
Script->>Script: validateForm
Script->>Popup: Enable_generateReport_when_valid
User->>Popup: Click_generateReport
Popup->>Script: generateReport
Script->>Storage: get github_token
Storage-->>Script: github_token_or_null
alt No_token_present
Script->>Popup: Render_token_warning_in_report_container
User->>Popup: Click_add_token_button
Popup->>Popup: Navigate_to_settings_html
else Token_present
Script->>GitHub: fetchGitHubData(token)
GitHub-->>Script: data_or_error
alt Fetch_success
Script->>Popup: renderReport
else Fetch_error
Script->>Popup: Show_error_message_in_report_container
end
end
File-Level Changes
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 2 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
container.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- The new
generateReporthandler is wired todocument.getElementById('generate-report-btn'), but the button inpopup.htmluses theid="generateReport", so the click listener will never be attached—these IDs should be made consistent. - The
generateReportfunction appears to reimplement report-generation and error rendering logic; consider reusing or refactoring any existing report-generation code to avoid duplication and keep behavior in one place. - The
#report-containerdiv is appended at the very end ofpopup.htmlafter all scripts, which may not be the intended UX location for error and token messages; it might be clearer to place this container near the report controls so the feedback appears contextually where the user interacts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `generateReport` handler is wired to `document.getElementById('generate-report-btn')`, but the button in `popup.html` uses the `id="generateReport"`, so the click listener will never be attached—these IDs should be made consistent.
- The `generateReport` function appears to reimplement report-generation and error rendering logic; consider reusing or refactoring any existing report-generation code to avoid duplication and keep behavior in one place.
- The `#report-container` div is appended at the very end of `popup.html` after all scripts, which may not be the intended UX location for error and token messages; it might be clearer to place this container near the report controls so the feedback appears contextually where the user interacts.
## Individual Comments
### Comment 1
<location path="src/scripts/popup.js" line_range="74-79" />
<code_context>
+ generateBtn.disabled = !isValid;
+ }
+
+ // Attach listeners
+ usernameInput.addEventListener("input", validateForm);
+ fromDateInput.addEventListener("change", validateForm);
+ toDateInput.addEventListener("change", validateForm);
// Dark mode setup
const darkModeToggle = document.querySelector('img[alt="Night Mode"]');
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider invoking `validateForm` once on load to ensure the disabled state matches any prefilled values.
Because the button is only toggled in response to `input`/`change` events, any prefilled or autofilled values won’t update its state and it may stay disabled until the user edits a field. Calling `validateForm()` once after registering the listeners will align the initial button state with the current field values.
```suggestion
// Attach listeners
usernameInput.addEventListener("input", validateForm);
fromDateInput.addEventListener("change", validateForm);
toDateInput.addEventListener("change", validateForm);
// Initialize button state based on any prefilled/autofilled values
validateForm();
// Dark mode setup
const darkModeToggle = document.querySelector('img[alt="Night Mode"]');
```
</issue_to_address>
### Comment 2
<location path="src/scripts/popup.js" line_range="232" />
<code_context>
container.innerHTML = `<p style="color:red;">Error fetching data: ${err.message}</p>`;
</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="232" />
<code_context>
container.innerHTML = `<p style="color:red;">Error fetching data: ${err.message}</p>`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `container.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.
Comment on lines
+74
to
79
| // Attach listeners | ||
| usernameInput.addEventListener("input", validateForm); | ||
| fromDateInput.addEventListener("change", validateForm); | ||
| toDateInput.addEventListener("change", validateForm); | ||
| // Dark mode setup | ||
| const darkModeToggle = document.querySelector('img[alt="Night Mode"]'); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider invoking validateForm once on load to ensure the disabled state matches any prefilled values.
Because the button is only toggled in response to input/change events, any prefilled or autofilled values won’t update its state and it may stay disabled until the user edits a field. Calling validateForm() once after registering the listeners will align the initial button state with the current field values.
Suggested change
| // Attach listeners | |
| usernameInput.addEventListener("input", validateForm); | |
| fromDateInput.addEventListener("change", validateForm); | |
| toDateInput.addEventListener("change", validateForm); | |
| // Dark mode setup | |
| const darkModeToggle = document.querySelector('img[alt="Night Mode"]'); | |
| // Attach listeners | |
| usernameInput.addEventListener("input", validateForm); | |
| fromDateInput.addEventListener("change", validateForm); | |
| toDateInput.addEventListener("change", validateForm); | |
| // Initialize button state based on any prefilled/autofilled values | |
| validateForm(); | |
| // Dark mode setup | |
| const darkModeToggle = document.querySelector('img[alt="Night Mode"]'); |
| .then(renderReport) | ||
| .catch(err => { | ||
| const container = document.getElementById('report-container'); | ||
| container.innerHTML = `<p style="color:red;">Error fetching data: ${err.message}</p>`; |
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
| .then(renderReport) | ||
| .catch(err => { | ||
| const container = document.getElementById('report-container'); | ||
| container.innerHTML = `<p style="color:red;">Error fetching data: ${err.message}</p>`; |
Contributor
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a container.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
Contributor
|
@Kathyayani-15 |
Member
|
Thanks @Kathyayani-15 for your contribution. This bug is already being addressed in #332, so I’m closing this PR to avoid duplication. We don’t want contributors spending effort on issues that are already in progress or resolved. Please feel free to pick up another issue from the issues list and continue contributing. |
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
3 participants
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 # (Use "Fixes", "Closes", or "Resolves" for automatic closing)
📝 Summary of Changes
Disabled the Generate Report button by default.
Added real-time validation for:
GitHub username (must not be empty)
Date range (From date must be ≤ To date)
The button is enabled only when all required fields are valid.
Prevents empty/invalid report generation and improves overall UX.
📸 Screenshots / Demo (if UI-related)
✅ Checklist
👀 Reviewer Notes
The validation logic is implemented in popup.js using event listeners on username and date inputs.
No existing report-generation logic was modified — only button enable/disable behavior was added to improve UX and prevent invalid submissions.
Summary by Sourcery
Add input validation and token checks before allowing report generation from the popup.
New Features:
Enhancements: