Compatibility changes for gekko and blink based browsers #181
Conversation
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
Reviewer's GuideThis PR introduces a cross-browser storage abstraction using a browser/polyfill approach, refactors all chrome.* callbacks into async/await browser API calls, and updates manifest, dependencies, and HTML to support Firefox (Gecko) and Blink-based browsers consistently. Class diagram for unified storage abstraction and usageclassDiagram
class browser {
+storage
+runtime
+tabs
}
class storage {
+local
}
class local {
+get(keys, callback)
+set(items, callback)
+remove(keys, callback)
}
class scrumHelper {
+storageGet(keys)
+storageSet(items)
+storageRemove(keys)
+getStorageData()
+saveToStorage(data, subject)
+loadFromStorage()
}
browser o-- storage
storage o-- local
scrumHelper ..> browser : uses
scrumHelper ..> storage : uses
scrumHelper ..> local : uses
class main {
+handleBodyOnLoad()
+handleEnableChange()
+handleStartingDateChange()
+handleEndingDateChange()
+handleLastWeekContributionChange()
+handleYesterdayContributionChange()
+handleGithubUsernameChange()
+handleGithubTokenChange()
+handleProjectNameChange()
+handleCacheInputChange()
+handleOpenLabelChange()
+handleUserReasonChange()
+handleShowCommitsChange()
}
main ..> browser : uses
main ..> storage : uses
main ..> local : uses
File-Level Changes
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 @Saksham-Sirohi - I've reviewed your changes - here's some feedback:
- The allIncluded function has grown very large and handles multiple responsibilities—consider refactoring it into smaller, purpose-driven helper functions to improve readability and maintainability.
- Storage calls are still mixed between direct browser.storage.local and the new storageGet/storageSet wrappers—unify all storage interactions through the wrapper API for consistency.
- There’s repeated DOM selection and event-binding logic across main.js and popup.js—extract common utilities or modules to reduce duplication and simplify future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The allIncluded function has grown very large and handles multiple responsibilities—consider refactoring it into smaller, purpose-driven helper functions to improve readability and maintainability.
- Storage calls are still mixed between direct browser.storage.local and the new storageGet/storageSet wrappers—unify all storage interactions through the wrapper API for consistency.
- There’s repeated DOM selection and event-binding logic across main.js and popup.js—extract common utilities or modules to reduce duplication and simplify future changes.
## Individual Comments
### Comment 1
<location> `src/scripts/scrumHelper.js:80` </location>
<code_context>
+ async function getStorageData() {
</code_context>
<issue_to_address>
getStorageData is called but not awaited, which may cause race conditions.
This could result in inconsistent state or errors. Please await getStorageData or restructure the function to ensure correct execution order.
</issue_to_address>
### Comment 2
<location> `src/scripts/scrumHelper.js:175` </location>
<code_context>
+ showCommits = false;
+ }
+ if (!items.showOpenLabel) {
+ showOpenLabel = false;
+ pr_open_button = '';
+ issue_opened_button = '';
+ pr_merged_button = '';
+ issue_closed_button = '';
+ }
+ if (items.githubCache) {
</code_context>
<issue_to_address>
Inconsistent variable naming for PR open button.
`pr_open_button` is set here, but the variable is previously defined as `pr_unmerged_button`. Please use consistent naming to avoid reference errors or UI issues.
</issue_to_address>
### Comment 3
<location> `src/scripts/scrumHelper.js:265` </location>
<code_context>
-
- function saveToStorage(data, subject = null) {
+ async function saveToStorage(data, subject = null) {
const cacheData = {
data: data,
</code_context>
<issue_to_address>
saveToStorage does not propagate errors to callers.
Currently, errors are only logged and a boolean is returned. Consider throwing errors or implementing a clearer error-handling strategy so callers are properly informed of failures.
Suggested implementation:
```javascript
async function saveToStorage(data, subject = null) {
try {
const cacheData = {
data: data,
cacheKey: githubCache.cacheKey,
hasSubject: !!subject,
};
// Assuming storageSet is the function to save to storage
await storageSet(cacheData, subject);
} catch (error) {
// Optionally log the error here if desired
// console.error('Failed to save to storage:', error);
throw error;
}
```
- If `storageSet` is not the correct function to save to storage, replace it with the actual storage-saving function.
- Update all callers of `saveToStorage` to handle thrown errors (e.g., with try/catch).
- Remove any code that expects a boolean return value from `saveToStorage`, as it will now throw on error.
</issue_to_address>
### Comment 4
<location> `src/scripts/scrumHelper.js:1138` </location>
<code_context>
allIncluded('email');
- $('button>span:contains(New conversation)').parent('button').click(() => {
- allIncluded();
+ document.querySelectorAll('button').forEach(button => {
+ if (button.textContent.includes('New conversation')) {
+ button.addEventListener('click', () => allIncluded());
+ }
});
}
</code_context>
<issue_to_address>
Selector for 'New conversation' button may be too broad.
This approach may bind events to unintended buttons if similar text appears elsewhere. Use a more precise selector or add checks to ensure only the intended button is targeted.
Suggested implementation:
```javascript
document.querySelectorAll('button').forEach(button => {
if (
button.textContent.trim() === 'New conversation' &&
button.closest('.conversation-toolbar') // Adjust this selector to match the actual parent/container
) {
button.addEventListener('click', () => allIncluded());
}
});
```
- Replace `.conversation-toolbar` with the actual class or selector of the container that uniquely identifies the "New conversation" button, if different.
- If the button has a unique attribute (e.g., `data-testid="new-conversation"`), use `document.querySelector('button[data-testid="new-conversation"]')` instead for even more precision.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Saksham-Sirohi
force-pushed
the
firefox-compatibility-addition
branch
11 times, most recently
from
13d4467 to
423fc91
Compare
Saksham-Sirohi
force-pushed
the
firefox-compatibility-addition
branch
from
423fc91 to
ea6d19d
Compare
|
sorry there was a lot changes detected due to my secondary editor's config which was causing sorcery to spit so many things out |
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.
Solves Chore #120 #135
Summary by Sourcery
Enable cross-browser compatibility by introducing browser-polyfill, replacing chrome.* calls with async browser.* methods, updating scripts and manifest for Firefox support, and adding the polyfill dependency
New Features:
Enhancements:
Build: