-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update to Chrome Extension Manifest v3 #20
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); |
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.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
// Create and store a new session | ||
sessionData = { | ||
session_id: currentTimeInMs.toString(), | ||
timestamp: currentTimeInMs.toString() |
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 |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
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: