-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Timeline custom filters #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces advanced filtering capabilities for pull requests with a comprehensive multi-tier data fetching strategy. The implementation adds a popup UI for filter customization, dynamic query construction for GitHub's search APIs, and robust fallback mechanisms for data retrieval.
- Added custom filter controls for date range, PR status, author type, and result limits with persistent storage
- Implemented multi-tier data fetching: web scraping → Events API → Search API with graceful fallbacks
- Enhanced UI with dark mode support, status indicators, and GitHub-native styling
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| firefox/popup.js, chrome/popup.js | New filter management logic with validation and storage |
| firefox/popup.html, chrome/popup.html | Filter UI with GitHub-themed styling and dark mode support |
| firefox/content.js, chrome/content.js | Refactored to support dynamic filters and multi-tier fetching |
| firefox/api.js, chrome/api.js | Added query builders and Events API fallback |
| firefox/ui.js, chrome/ui.js | Updated empty state message to reflect filters |
| README.md | Documentation updates for new filtering features |
Comments suppressed due to low confidence (2)
firefox/content.js:15
- Unused variable storage.
const storage = typeof browser !== 'undefined' ? browser.storage : chrome.storage;
firefox/api.js:232
- The value assigned to dateString here is unused.
dateString = dateElement.getAttribute('datetime') ||
dateElement.getAttribute('title') ||
dateElement.textContent;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (typeof chrome !== 'undefined' && chrome.tabs) { | ||
| chrome.tabs.query({active: true, currentWindow: true}, function(tabs) { | ||
| if (tabs[0] && tabs[0].url && tabs[0].url.includes('github.com')) { | ||
| chrome.tabs.executeScript(tabs[0].id, { |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chrome.tabs.executeScript API is deprecated in Manifest V3 and should be replaced with chrome.scripting.executeScript. If targeting Manifest V3, update this to use the scripting API with proper permissions.
| if (typeof chrome !== 'undefined' && chrome.tabs) { | ||
| chrome.tabs.query({active: true, currentWindow: true}, function(tabs) { | ||
| if (tabs[0] && tabs[0].url && tabs[0].url.includes('github.com')) { | ||
| chrome.tabs.executeScript(tabs[0].id, { |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chrome.tabs.executeScript API is deprecated in Manifest V3 and should be replaced with chrome.scripting.executeScript. If targeting Manifest V3, update this to use the scripting API with proper permissions.
| // Fetch user PRs by scraping GitHub's issues page (same as web interface) | ||
| const allPRs = await fetchPRsFromWebInterface(username); | ||
| // Cross-browser storage API | ||
| const storage = typeof browser !== 'undefined' ? browser.storage : chrome.storage; |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent browser API usage. Line 15 defines a storage variable for cross-browser compatibility, but line 20 directly uses browser.storage instead of the defined storage variable, making the fallback mechanism ineffective.
| // Load user filters | ||
| let userFilters; | ||
| try { | ||
| const result = await browser.storage.sync.get({ filters: null }); |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent browser API usage. Line 15 defines a storage variable for cross-browser compatibility, but line 20 directly uses browser.storage instead of the defined storage variable, making the fallback mechanism ineffective.
| const result = await browser.storage.sync.get({ filters: null }); | |
| const result = await storage.sync.get({ filters: null }); |
| }; | ||
|
|
||
| // Theme detection function | ||
| function initializeThemeDetection() { |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initializeThemeDetection function is defined but never called. Either invoke this function in the DOMContentLoaded event handler (line 257) or remove it if theme detection is handled automatically by CSS media queries.
| fill="black"></path> | ||
| </svg> | ||
| <p class="text">Created by LeleDallas</p> | ||
| <p class="text" style="color: black;">Created by LeleDallas</p> |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded black color in inline styles overrides theme CSS variables, breaking dark mode support. The SVG path and text will remain black in dark mode. Remove the inline styles and use CSS variables like var(--color-fg-default) for proper theme support.
| // Handle status filters | ||
| const statusFilters = []; | ||
| if (filters.showOpen && !filters.showDraft) { | ||
| statusFilters.push('state:open', '-is:draft'); // Exclude drafts |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The status filter logic in buildGitHubSearchURL uses a different implementation pattern than the chrome version. Firefox pushes two separate array elements ('state:open', '-is:draft') while Chrome pushes them as separate statements. Consider using the same pattern (separate push statements) for consistency across both browser implementations.
| statusFilters.push('state:open', '-is:draft'); // Exclude drafts | |
| statusFilters.push('state:open'); // Exclude drafts | |
| statusFilters.push('-is:draft'); |
| // Try to extract date information from the page | ||
| const dateElement = item.querySelector('time, [datetime], .opened-by'); | ||
| let dateString = null; | ||
| if (dateElement) { | ||
| dateString = dateElement.getAttribute('datetime') || | ||
| dateElement.getAttribute('title') || | ||
| dateElement.textContent; | ||
| } |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extracted dateString variable is never used after being set. Either remove this code block or use the date information for filtering or sorting purposes.
| // Get the effective max results value | ||
| function getMaxResults(filters) { | ||
| if (filters.maxResults === 'custom') { | ||
| return Number.parseInt(filters.customMaxResults) || 8; |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing radix parameter in Number.parseInt. Always specify the radix (10) to avoid potential parsing issues: Number.parseInt(filters.customMaxResults, 10).
| return Number.parseInt(filters.customMaxResults) || 8; | |
| return Number.parseInt(filters.customMaxResults, 10) || 8; |
| // Get the effective max results value | ||
| function getMaxResults(filters) { | ||
| if (filters.maxResults === 'custom') { | ||
| return Number.parseInt(filters.customMaxResults, 10) || 8; |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent implementation between firefox and chrome versions. Firefox version (line 450) is missing the radix parameter while Chrome version correctly includes it. Ensure both implementations are identical.
m-aguilar9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for incorporating the filtering feature!
Glad that helped!😄 |
This pull request introduces advanced filtering capabilities and a robust multi-tier data fetching strategy to the GitHub Activity extension, along with major improvements to reliability, user experience, and code maintainability. The changes span both documentation and implementation, providing users with granular control over which pull requests are displayed and ensuring the extension remains functional even if GitHub’s web interface changes.
Preview
List of changes proposed in this pull request