feat: introduce GitHubHelper module as first step toward SCM service layer#507
feat: introduce GitHubHelper module as first step toward SCM service layer#507being-iota wants to merge 5 commits intofossasia:mainfrom
Conversation
…layer This PR introduces a basic GitHubHelper module as a first step toward modularizing GitHub-specific logic. Currently, GitHub-related functionality is tightly coupled within scrumHelper.js, which makes it difficult to extend support to other SCM platforms like GitLab or Bitbucket. Changes: - Add src/scripts/githubHelper.js with GitHubHelper class - Move fetchUserRepositories into GitHubHelper.fetchUserRepositories() - Expose window.gitHubHelper instance and backward-compat shim for window.fetchUserRepositories so popup.js needs zero changes - Register githubHelper.js in popup.html and both manifests before scrumHelper.js This change lays the foundation for extracting GitHub logic into a reusable service layer, enabling future multi-SCM support.
Reviewer's GuideExtracts GitHub-specific repo fetching logic from scrumHelper into a new reusable GitHubHelper service module, wires it into the extension bootstrap, and preserves backward-compatible globals for existing popup behavior. Sequence diagram for GitHubHelper.fetchUserRepositories flowsequenceDiagram
actor User
participant PopupUI as popupJs
participant GitHubHelper as gitHubHelper
participant BrowserStorage as browser_storage
participant GitHubREST as github_search_api
participant GitHubGraphQL as github_graphql_api
User->>PopupUI: trigger repo filtering
PopupUI->>GitHubHelper: fetchUserRepositories(username, token, org)
GitHubHelper->>BrowserStorage: storage.local.get(startingDate, endingDate, yesterdayContribution)
BrowserStorage-->>GitHubHelper: date range settings
GitHubHelper->>GitHubREST: GET /search/issues (author query)
GitHubHelper->>GitHubREST: GET /search/issues (commenter query)
GitHubREST-->>GitHubHelper: issues and comments
GitHubHelper->>GitHubGraphQL: POST /graphql (batched repo queries)
GitHubGraphQL-->>GitHubHelper: repo metadata
GitHubHelper-->>PopupUI: sorted list of repositories
PopupUI-->>User: render repo list
note over PopupUI,GitHubHelper: window.fetchUserRepositories(username, token, org) calls gitHubHelper.fetchUserRepositories
Class diagram for new GitHubHelper service moduleclassDiagram
class GitHubHelper {
+fetchUserRepositories(username, token, org) Promise
}
class WindowGlobal {
+GitHubHelper
+gitHubHelper
+fetchUserRepositories(username, token, org) Promise
}
GitHubHelper <.. WindowGlobal : constructor reference
GitHubHelper <.. WindowGlobal : gitHubHelper instance
GitHubHelper <.. WindowGlobal : fetchUserRepositories delegation
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}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- GitHubHelper currently depends directly on
browser.storage.localandwindow, which makes it harder to reuse or unit-test; consider passing storage and environment objects into the constructor so the helper is less tightly coupled to the extension runtime. - The
fetchUserRepositoriesmethod hard-codes a limit of 50 repos fromrepoNames.slice(0, 50); it would be clearer to extract this into a named constant or parameter and document the trade-off so future changes don’t inadvertently change behavior. - The global exports block mixes CommonJS (
module.exports) with assignment towindow.*; to avoid surprising behavior in different environments, consider centralizing this in a small wrapper or using a single export strategy with a thin browser-specific bootstrap script.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- GitHubHelper currently depends directly on `browser.storage.local` and `window`, which makes it harder to reuse or unit-test; consider passing storage and environment objects into the constructor so the helper is less tightly coupled to the extension runtime.
- The `fetchUserRepositories` method hard-codes a limit of 50 repos from `repoNames.slice(0, 50)`; it would be clearer to extract this into a named constant or parameter and document the trade-off so future changes don’t inadvertently change behavior.
- The global exports block mixes CommonJS (`module.exports`) with assignment to `window.*`; to avoid surprising behavior in different environments, consider centralizing this in a small wrapper or using a single export strategy with a thin browser-specific bootstrap script.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Inject storage via constructor (storage = browser.storage.local) to remove tight coupling and enable testing without a real browser - Replace hardcoded .slice(0, 50) with MAX_REPOS constant - Add clarifying comment on dual export (browser + Node compatibility)
|
Thanks for the contribution! A process note. We have automatic Copilot PR reviews enabled on this repository. These reviews are only triggered if the contributor has GitHub Copilot enabled and an active license on their own account. Please enable Copilot in your GitHub settings if you have access. In many regions, free licenses are available through educational institutions or developer programs. Enabling Copilot helps us speed up the auto review process and reduces manual review overhead for the core team. |
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Please resolve conflicts and address AI reviews.
There was a problem hiding this comment.
Pull request overview
Introduces a new GitHubHelper module to begin separating GitHub-specific logic from scrumHelper.js, while keeping existing popup behavior working via a backward-compat window.fetchUserRepositories shim.
Changes:
- Added
src/scripts/githubHelper.jswith aGitHubHelperclass andfetchUserRepositories()implementation plus a global shim/instance. - Removed the inline
fetchUserRepositoriesimplementation fromsrc/scripts/scrumHelper.js. - Registered
githubHelper.jsto load beforescrumHelper.jsinpopup.htmland both browser manifests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/scripts/scrumHelper.js | Removes the embedded GitHub repo-fetching function and relies on the helper-provided shim. |
| src/scripts/githubHelper.js | Adds a GitHub-specific helper class and global instance/shim for repository discovery. |
| src/popup.html | Loads githubHelper.js before scrumHelper.js so the shim is available. |
| src/manifests/firefox.json | Ensures githubHelper.js is loaded before scrumHelper.js in content scripts. |
| src/manifests/chrome.json | Ensures githubHelper.js is loaded before scrumHelper.js in content scripts. |
| .vscode/settings.json | Adds a (currently empty) VS Code workspace settings file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * This is the first step toward a generic SCM service layer — mirroring the | ||
| * existing GitLabHelper pattern so scrumHelper.js can eventually call either | ||
| * platform through a unified interface. |
There was a problem hiding this comment.
The header comment says this helper is “mirroring the existing GitLabHelper pattern”, but GitLabHelper currently only exposes the class (no shared instance/shim) and is instantiated inside scrumHelper.js. Please update this comment to reflect the actual pattern being used here (or align the implementations) to avoid misleading future refactors toward the SCM service layer.
| * This is the first step toward a generic SCM service layer — mirroring the | |
| * existing GitLabHelper pattern so scrumHelper.js can eventually call either | |
| * platform through a unified interface. | |
| * This extracts GitHub-specific behavior into a helper class and, in the | |
| * browser context, exposes a shared instance plus a backward-compatibility | |
| * shim for existing callers in popup.js and scrumHelper.js. |
| * @param {object} storage - Storage backend (defaults to browser.storage.local). | ||
| * Injected to avoid tight coupling and enable testing without a real browser env. | ||
| */ | ||
| constructor(storage = browser.storage.local) { |
There was a problem hiding this comment.
constructor(storage = browser.storage.local) will throw a ReferenceError in Node/test environments where browser is undefined, which undermines the “Node.js (test) compatibility” note later in the file. Consider defaulting via globalThis.browser?.storage?.local (or requiring an injected storage in non-extension contexts) so new GitHubHelper() is safe outside WebExtension runtimes.
| constructor(storage = browser.storage.local) { | |
| constructor(storage = globalThis.browser?.storage?.local) { |
| { | ||
| } No newline at end of file |
There was a problem hiding this comment.
This workspace settings file is currently an empty object and has no effect. If there are no intended VS Code settings to commit, consider removing it to avoid adding noise / implying editor-specific configuration.
| { | |
| } |
…ection, extract MAX_REPOS - Change constructor default to globalThis.browser?.storage?.local for safer optional chaining - Replace browser.storage.local.get with this.storage.get throughout - Update header comment to describe actual module purpose - Remove .vscode/settings.json from tracking
Kept our version: window.fetchUserRepositories is now provided by githubHelper.js as a backward-compat shim, so the old global assignment in scrumHelper.js is intentionally removed.
|
Hi, @mariobehling!
Ready for your review! |
Hi @being-iota, my proposed solution as part of GSOC in the issue #495 is much similar to your approach in this PR. Implementing a service layer/shared interface which is independent of the SCM platform would be the best option, as it would allow easier addition of new SCM platforms in the future, like GitLab, Gitea, bitBucket and more... Congrats on this PR though! I am assuming that you have already verified (Locally) that the extension works well after the proposed changes in #507, but still, I would love to provide a secondary verification to help get this PR merged soon! I am currently testing it locally by cloning from https://github.com/being-iota/scrum_helper/tree/feat/github-helper-module In a while, I would share screenshots of the running extension here in the comments to help provide an additional verification about extension working fine locally, and hopefully this would be merged by @mariobehling soon. |
|
Hi @mariobehling and @being-iota, I have locally tested the extension after the changes that was done by @being-iota. Here are the Results: Needs fix: "Insert to Email Button" only, everything else is working. Please refer to the screenshots attached to see the results:
@mariobehling and @being-iota, Tomorrow I will try to figure out why the "Insert to Email" button is not working, and help resolve it. |
This PR introduces a basic GitHubHelper module as a first step toward modularizing GitHub-specific logic.
Currently, GitHub-related functionality is tightly coupled within scrumHelper.js, which makes it difficult to extend support to other SCM platforms like GitLab or Bitbucket.
Changes:
This change lays the foundation for extracting GitHub logic into a reusable service layer, enabling future multi-SCM support.
Fixes
Part of #495
Summary by Sourcery
Extract GitHub-specific repository fetching into a dedicated GitHubHelper service and wire it into the extension while preserving backwards compatibility.
Enhancements: