Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Extension: Fix multiple inits (onload) #307

Closed
wants to merge 2 commits into from
Closed

Extension: Fix multiple inits (onload) #307

wants to merge 2 commits into from
+21 −30

Conversation

yoyurec
Copy link
Contributor

@yoyurec yoyurec commented Apr 30, 2022 •
edited
Loading

Fixes #295
Fixes #177

Please be aware chrome.tabs.onUpdated will also fired for iframes, if a page contains many iframes, each completed iframe will trigger the event though you have checked changeInfo.status. Use chrome.webNavigation.onCompleted

@yoyurec yoyurec mentioned this pull request Apr 30, 2022
Closed
@yoyurec
Copy link
Contributor Author

yoyurec commented Apr 30, 2022

Cannot get chrome.webNavigation because property webNavigation is missing in object type [1]. [prop-missing] node_modules/flow-interfaces-chrome/interfaces/Chrome.js

em... )
too old package
https://github.com/Shraymonks/flow-interfaces-chrome/search?q=webNavigation

@yoyurec yoyurec mentioned this pull request May 1, 2022
Closed
karlicoss added a commit that referenced this pull request May 1, 2022
@karlicoss
tests: add end2end test for sidebar flickering on some sites
4eb5286 
see #295

should be fixed by #307
karlicoss added a commit that referenced this pull request May 1, 2022
@karlicoss
tests: add end2end test for sidebar flickering on some sites
7e6d8fe 
see #295

should be fixed by #307
karlicoss added a commit that referenced this pull request May 1, 2022
@karlicoss
tests: add end2end test for sidebar flickering on some sites
c909216 
see #295

should be fixed by #307
karlicoss
karlicoss reviewed May 1, 2022
View reviewed changes
Copy link
Owner

@karlicoss karlicoss left a comment

Choose a reason for hiding this comment

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

So I had an error in the log, in particular, 'always show sidebar' stoppedworking
So I think this requires more changes since we switched from the Tab type to oncompleted callback -- I committed Flow types to make the errors more obvious

extension/src/background.js
const url = unwrap(tab.url);
const tabId = unwrap(tab.id);
const tabId = unwrap(tab.tabId);
Copy link
Owner

Choose a reason for hiding this comment

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

hmm, looks like this is causing these for me

requesting! https://news.ycombinator.com/item?id=31224707
common.js:19 Uncaught (in promise) Error: undefined or null!
    at unwrap (common.js:19:11)
    at shouldProcessPage (background.js:583:62)
    at openSidebar (background.js:702:24)
    at eval (background.js:248:22)

extension/src/background.js

// also if you, say, go to web.telegram.org it's gonna show multiple notifications due to redirect... but perhaps this can just be suppressed..
if (info.frameId !== 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

should this check go first (before the URL check)? would make it faster and less logging

extension/src/background.js Outdated
delete info.favIconUrl
//
console.debug('onUpdated %o %o', tab, info)
// $FlowExpectedError
Copy link
Owner

Choose a reason for hiding this comment

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

yeah, looks like they are incomplete for these Shraymonks/flow-interfaces-chrome#1 (comment)

@yoyurec
Copy link
Contributor Author

yoyurec commented May 8, 2022

any updates with checking functionality here?

@karlicoss
Copy link
Owner

ah have you seen CI logs for build_extension pipeline? (not sure you have the permissions) -- otherwise you can try running npm run flow locally (just make sure to pull my latest commits, where I annotated the function properly)

I think there are more changes that need to be done -- the object we are now passing into onTabLoaded here

Haven't checked exactly what's missing though -- it may be possible it's just a few occasions of replacing .id with .tabId like here?

promnesia/extension/src/background.js

Lines 529 to 530 in f076f1c

const url = unwrap(tab.url)
const tid = unwrap(tab.id)

@yoyurec
Copy link
Contributor Author

yoyurec commented Jul 26, 2022

so what's the status? will this PR be used somehow or you have found another solution?

@purarue purarue mentioned this pull request Oct 10, 2022
Open
@karlicoss
Copy link
Owner

Hey, @yoyurec sorry for late reply -- as I mention in the comment #307 (comment) , there are still some outstanding issues -- but otherwise I haven't look at it

karlicoss added a commit that referenced this pull request Jan 7, 2023
@karlicoss
extension: switch to webnavigation.onCompleted instead of tabs.onUpdate
50901e0 
relevant:
- #177
- #295
- #307
karlicoss added a commit that referenced this pull request Jan 8, 2023
@karlicoss
extension: switch to webnavigation.onCompleted instead of tabs.onUpdate
46a2e34 
relevant:
- #177
- #295
- #307
@karlicoss
Copy link
Owner

closing in favor of #354 (thanks for the inspiration though :) )

@karlicoss karlicoss closed this Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karlicoss karlicoss karlicoss left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@yoyurec @karlicoss