-
-
Notifications
You must be signed in to change notification settings - Fork 4
Update to Chrome Extension Manifest v3 #20
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: master
Are you sure you want to change the base?
Conversation
oxipng -o max --dir oxipng --alpha *.png
…as the legacy 'page'
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.
Copilot reviewed 11 out of 20 changed files in this pull request and generated 2 comments.
Files not reviewed (9)
- _locales/en/messages.json: Language not supported
- manifest.json: Language not supported
- scripts/build: Language not supported
- scripts/clean: Language not supported
- scripts/optimize-icons: Language not supported
- src/bg/background.html: Language not supported
- TODO.md: Evaluated as low risk
- js/chromeExtensionApiAbstractions.js: Evaluated as low risk
- js/googleAnalytics.js: Evaluated as low risk
Comments suppressed due to low confidence (2)
src/google-analytics.js:121
- Ensure that the error object is a plain object before spreading it.
...error
src/google-analytics.js:103
- Handle network errors more gracefully in the fireEvent function.
console.error('Google Analytics request failed with an exception', e);
| */ | ||
| export async function setSettings(updater) { | ||
| const currentSettings = await getSettings(); | ||
| const newSettings = updater(currentSettings); |
Copilot
AI
Jan 29, 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.
Add a check to ensure that newSettings is not null or undefined before setting it in storage.
| // Create and store a new session | ||
| sessionData = { | ||
| session_id: currentTimeInMs.toString(), | ||
| timestamp: currentTimeInMs.toString() |
Copilot
AI
Jan 29, 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 timestamp should be stored as a number instead of a string.
| timestamp: currentTimeInMs.toString() | |
| timestamp: currentTimeInMs |
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.
const currentTimeInMs = Date.now(); contains a number, not a Date object; so calling .toString() on it isn't going to store the Date as a formatted string..
|
|
For current status/plans/etc, refer to that issue; for example, from this comment: