Skip to content

manifest_v3 updates#63

Merged
qredwoods merged 1 commit intoserenadeai:masterfrom
alscotty:master
May 8, 2025
Merged

manifest_v3 updates#63
qredwoods merged 1 commit intoserenadeai:masterfrom
alscotty:master

Conversation

@alscotty
Copy link
Copy Markdown
Contributor

@alscotty alscotty commented Mar 5, 2025

On 3/4/2025 (to present) I saw this warning on chrome extensions:
Screen Shot 2025-03-05 at 11 14 22 AM

Going through examples here: https://developer.chrome.com/docs/extensions/develop/migrate/checklist
I saw some room to for changes - mainly updating the setTimeouts to chrome alarm api

Testing - npm run dist, then turning off my existing serenade chrome plugin and load-unpacking mine, then:
npm run test - manually testing some voice commands, etc.
Links/tabs/clicks/scrolls etc. still working

@alscotty
Copy link
Copy Markdown
Contributor Author

@MattWiethoff Hi Matt! From discord you mentioned as someone who might be able to review this? Thanks in advance!

Copy link
Copy Markdown

@sombrafam sombrafam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Scott,

Thanks very much for have working on this. Since many of us are not familiar with JavaScript and chrome plugins, can you give an explanation in your pull request, of why the plugin stopped working, (what parts in the plugin we're affected by the Manifest update), and how your changes address the problem?

I added a few comments from things I noted from your commit.

Erlon

Comment thread manifest.json Outdated
Comment thread manifest.json Outdated
Comment thread manifest.json Outdated
Comment thread manifest.json Outdated
Comment thread src/editor.ts Outdated
Comment thread src/injected-command-handler.ts
@alscotty
Copy link
Copy Markdown
Contributor Author

alscotty commented Apr 22, 2025

@sombrafam Thanks for the comments, some of this can be cleaned up. If you're saying you don't understand JavaScript or plugins, is there someone else who can review this instead?
There's more context in the link provided: https://developer.chrome.com/docs/extensions/develop/migrate/checklist
You need to pass these requirements otherwise you get the warning that it doesn't follow best practices (see above screenshot)

@sombrafam
Copy link
Copy Markdown

The request is for a better rationale of the change, not for JavaScript itself which can be read and understood. But, now I see your first comment in the bug and I think we are good in that regard.

@alscotty
Copy link
Copy Markdown
Contributor Author

alscotty commented Apr 23, 2025

Thanks @sombrafam I've addressed those comments and re-tested. While I think we've addressed everything in https://developer.chrome.com/docs/extensions/develop/migrate/checklist
ultimately we will need to submit and get it re-reviewed by chrome extension team to get the greenlight/ensure they can remove the warning for us
Most of the changes are already done, this PR mainly changes timeouts to alarms

@sombrafam
Copy link
Copy Markdown

Hi Scott, there are still many changes not related to the issue, ill point them in the code.

Copy link
Copy Markdown

@sombrafam sombrafam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Scott,

Make sure you change only the lines that are relevant to the fix. Many of our changes are only formatting changes and they should not go in this patch.

Comment thread manifest.json
Comment thread manifest.json
Comment thread manifest.json
@alscotty
Copy link
Copy Markdown
Contributor Author

alscotty commented May 6, 2025

@sombrafam you are re-flagging issues from my first PR - these have all been fixed already (you are viewing an outdated version) Apart from 1 line that I changed just now. See https://github.com/serenadeai/chrome/pull/63/files for current code changes. For reference on any PR you are reviewing, you can click "Files changed" to see the most recent version of the changes.

@sombrafam
Copy link
Copy Markdown

Did you push -f the changes to your branch? If you navigate to '#63' -> 'Commits', you will see what I see. The commits there are not up to date. Your source repo is not right as well: https://github.com/alscotty/chrome/commits/master/

Can you squash those 3 last commits, do the fixes and update the pull request (just by push forcing a new master in your fork will update the PR).

@alscotty
Copy link
Copy Markdown
Contributor Author

alscotty commented May 7, 2025

I don't see what you see, as the commits are up to date. And I don't know what you mean by the source repo is "not right"? If you navigate to package.json you can see there's only 1 line of code changed - you commented on an old version since you can see the formatting stuff. I guess you want all the changes in 1 commit, but this is how most engineers use git - to incrementally save their work and explain their thought processes as they iterate and test.

@sombrafam
Copy link
Copy Markdown

sombrafam commented May 7, 2025

Hi Scott, Im glad your are doing this for the community and having the pacience to discuss this. This is what I mean and now I understand what is happening. You submitted the first change in commit f96b6f01ab0a0c348f1d248c21746980e9cee1be, the did the fixes in the follow up commits (5c27014a7ac3d01b7909783b94393bf536a46e8d and 1b46620a8481dc91c66de337da39dbb85457e422).

This is not right because you create multiple commits to revert something that shouldn't have been done in the first commit and your fixes wont be shown to reviewers. The way we do it, is that instead of creating a new commit, you ammend the first commit, and 'push -f' on top of your branch.

You can have multiple commits per pull request as long as the chains are self-contained. But in this case we wouldbe better to have all in a 1 commit like I did in PR #64.

migrate timers to alarms, testing checkpoint passed

update CSP in manifest, retested and passed

clean up

increment version number, install @types/chrome for typescript to use chrome apis

undo 1 line of formatting
@alscotty
Copy link
Copy Markdown
Contributor Author

alscotty commented May 7, 2025

done, squashed all changes to 1 commit

@qredwoods
Copy link
Copy Markdown
Collaborator

@MattWiethoff please review fix for chrome extension.

@qredwoods qredwoods mentioned this pull request May 8, 2025
@sombrafam
Copy link
Copy Markdown

We can close this sine is a duplicate from PR#64

@qredwoods qredwoods added the bug Something isn't working label May 8, 2025
@qredwoods
Copy link
Copy Markdown
Collaborator

Matt approved #64 but that was a duplicate of this one which has all the context and author history, so merging this one.

@qredwoods qredwoods merged commit 25b2531 into serenadeai:master May 8, 2025
@alscotty
Copy link
Copy Markdown
Contributor Author

alscotty commented May 8, 2025

Thanks all! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants