Check box state handling when token not present#384
Check box state handling when token not present#384Mateeb-Haider wants to merge 4 commits intofossasia:mainfrom
Conversation
Reviewer's GuideEnforces that the "Show commits on open/draft PRs" option is disabled when no GitHub token is present, adds matching token-warning UI/behavior, wires that validation into popup initialization and token input changes, and performs a small refactor of a repo filtering callback for readability. Sequence diagram for token-gated ShowCommits toggle behaviorsequenceDiagram
actor User
participant Popup as PopupUI
participant DOM as DOMElements
participant Storage as ChromeStorage
User->>Popup: Open_extension_popup
Popup->>Storage: get(darkMode, showCommits, githubToken, ...)
Storage-->>Popup: Settings_state
Popup->>DOM: initializePopup()
Popup->>DOM: checkTokenForFilter()
Popup->>DOM: checkTokenForShowCommits()
DOM->>DOM: evaluate showCommits.checked and githubTokenInput.value
alt showCommits enabled and no token
DOM->>DOM: uncheck showCommits
DOM->>Storage: set({showCommits: false})
DOM->>DOM: hide tokenWarningForShowCommits
else showCommits disabled or has token
DOM->>DOM: toggle tokenWarningForShowCommits(hidden)
end
User->>DOM: type_in_githubTokenInput
DOM->>DOM: checkTokenForFilter()
DOM->>DOM: checkTokenForShowCommits()
DOM->>DOM: recompute hasToken and warning_visibility
User->>DOM: click showCommitsCheckbox(change)
DOM->>DOM: hasToken = githubTokenInput.value.trim() !== ''
alt checkbox checked and no token
DOM->>DOM: uncheck showCommitsCheckbox
DOM->>DOM: show tokenWarningForShowCommits
DOM->>DOM: add shake-animation class
DOM->>DOM: remove shake-animation after 620ms
DOM->>DOM: hide tokenWarningForShowCommits after 3000ms
DOM->>Storage: set({showCommits: false})
else checkbox state valid
DOM->>Storage: set({showCommits: showCommitsCheckbox.checked})
end
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}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
showCommitsCheckboxchange handler reimplements most of the logic already incheckTokenForShowCommits; consider routing both the change event and token input updates through the same helper to avoid duplication and inconsistent behavior. - Both
checkTokenForShowCommitsand the checkbox change handler schedule timeouts to hide the warning; it may be cleaner to centralize the show/hide timing in one place and clear any existing timeout to avoid overlapping or flickering UI states.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `showCommitsCheckbox` change handler reimplements most of the logic already in `checkTokenForShowCommits`; consider routing both the change event and token input updates through the same helper to avoid duplication and inconsistent behavior.
- Both `checkTokenForShowCommits` and the checkbox change handler schedule timeouts to hide the warning; it may be cleaner to centralize the show/hide timing in one place and clear any existing timeout to avoid overlapping or flickering UI states.
## Individual Comments
### Comment 1
<location> `src/scripts/popup.js:681-698` </location>
<code_context>
}
}
showCommitsCheckbox.addEventListener('change', () => {
+ const hasToken = githubTokenInput.value.trim() !== '';
+
+ if (showCommitsCheckbox.checked && !hasToken) {
+ showCommitsCheckbox.checked = false;
+ const tokenWarning = document.getElementById('tokenWarningForShowCommits');
+ if (tokenWarning) {
+ tokenWarning.classList.remove('hidden');
+ tokenWarning.classList.add('shake-animation');
+ setTimeout(() => tokenWarning.classList.remove('shake-animation'), 620);
+ setTimeout(() => {
+ tokenWarning.classList.add('hidden');
+ }, 3000);
+ }
+ }
+
chrome?.storage.local.set({ showCommits: showCommitsCheckbox.checked });
});
githubTokenInput.addEventListener('input', () => {
</code_context>
<issue_to_address>
**suggestion:** Deduplicate the token warning logic between `checkTokenForShowCommits` and the `showCommitsCheckbox` change handler.
This handler now reimplements similar logic with different timing and animation, which risks inconsistent behavior over time. Consider invoking `checkTokenForShowCommits` here (or extracting a shared helper for warning display, duration, and animation) so the UX is defined in one place and easier to maintain.
```suggestion
showCommitsCheckbox.addEventListener('change', () => {
// Delegate token validation and warning display to a single place
if (showCommitsCheckbox.checked) {
checkTokenForShowCommits();
// If token validation failed, checkTokenForShowCommits is responsible
// for restoring the checkbox state and showing any warnings.
if (!showCommitsCheckbox.checked) {
chrome?.storage.local.set({ showCommits: false });
return;
}
}
chrome?.storage.local.set({ showCommits: showCommitsCheckbox.checked });
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This pull request implements token enforcement for the "Show commits on open PRs/draft PRs" checkbox to prevent users from enabling the feature without a GitHub token, matching the existing pattern used for the repository filtering feature.
Changes:
- Added
checkTokenForShowCommits()function to validate token availability and auto-disable the feature when no token is present - Integrated validation checks at initialization, token input changes, and checkbox toggle events
- Added warning UI element with appropriate styling and i18n support for user feedback
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/scripts/popup.js | Implements checkTokenForShowCommits() function, adds event listeners for validation, and includes unrelated code formatting fix for repository filter logic |
| src/popup.html | Adds warning element tokenWarningForShowCommits with consistent styling below the show commits checkbox |
| src/_locales/en/messages.json | Adds tokenRequiredShowCommitsWarning i18n key for the warning message |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Mateeb-Haider
changed the title
|
Hi @vedansh-5 @hpdang , |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "tokenRequiredShowCommitsWarning": { | ||
| "message": "A GitHub token is required to show commits on open or draft PRs. Please add one in the settings.", | ||
| "description": "Warning message shown when show commits is enabled without a token." | ||
| }, |
There was a problem hiding this comment.
The new i18n key tokenRequiredShowCommitsWarning has been added only to the English locale. Consider adding translations for other supported locales (de, es, fr, he, hi, id, it, ja, ml, nb, pt, pt_BR, ru, te, uk, vi, zh_CN) to maintain consistency with the existing tokenRequiredWarning key, which exists in all locales. If translations are typically added in separate PRs or by translation contributors, this can be tracked as a follow-up task.
| } | ||
|
|
||
| const tokenWarning = document.getElementById('tokenWarningForShowCommits'); | ||
| if (tokenWarning) { |
There was a problem hiding this comment.
When hiding the warning due to token presence or feature being disabled, the existing timeout stored in showCommitsWarningTimeout should be cleared to prevent unnecessary timer execution. Add a check to clear the timeout before hiding the warning, similar to how it's handled in showTokenWarningForShowCommits.
| if (tokenWarning) { | |
| if (tokenWarning) { | |
| if (showCommitsWarningTimeout) { | |
| clearTimeout(showCommitsWarningTimeout); | |
| showCommitsWarningTimeout = null; | |
| } |
src/scripts/popup.js
Outdated
| const filtered = availableRepos.filter((repo) => { | ||
| if (selectedRepos.includes(repo.fullName)) { | ||
| return false; | ||
| } | ||
| ); | ||
| if (!query) { | ||
| return true; | ||
| } | ||
| return repo.name.toLowerCase().includes(query) || repo.description?.toLowerCase().includes(query); | ||
| }); |
There was a problem hiding this comment.
This formatting change appears to be unrelated to the PR's stated purpose of adding token enforcement for the "Show commits" checkbox. While the reformatting improves readability, it would be better to keep unrelated changes in a separate commit or PR to maintain clear change history.
Mateeb-Haider
force-pushed
the
feat/require-token-for-show-commits
branch
from
a6424f3 to
2f117cf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/scripts/popup.js
Outdated
| animateWarning: true, | ||
| warningDurationMs: 3000, | ||
| }); | ||
| chrome?.storage.local.set({ showCommits: showCommitsCheckbox.checked }); |
There was a problem hiding this comment.
There's a race condition in this event handler. When the user checks the "Show commits" checkbox without a token, checkTokenForShowCommits() sets showCommits.checked = false and saves to storage. However, line 718 then unconditionally saves showCommitsCheckbox.checked to storage, which could overwrite the corrected value with true depending on timing.
This should follow the same pattern as the useRepoFilter checkbox (lines 942-1065), where storage is only updated after validation passes. The chrome?.storage.local.set() call should be moved inside checkTokenForShowCommits() or only executed when validation passes, similar to how useRepoFilter returns early at line 976 if validation fails.
| chrome?.storage.local.set({ showCommits: showCommitsCheckbox.checked }); | |
| const checked = showCommitsCheckbox.checked; | |
| chrome?.storage.local.set({ showCommits: checked }); |
|
@vedansh-5 PTAL |
|
Please pull in the latest changes. |
📌 Fixes
Closes #313
📝 Summary of Changes
Related issue: #313
📸 Screenshots / Demo (if UI-related)
Screencast.from.2026-02-19.23-55-42.webm
✅ Checklist
👀 Reviewer Notes
Summary by Sourcery
Enforce GitHub token requirements for the "Show commits" option in the popup and align its UX with existing token-gated behaviors.
Bug Fixes:
Enhancements:
Documentation: