latestchatty / chromeshack

Browser extension containing various scripts that enhance the Shacknews comments
http://adam.hughes.cc/shack/chromeshack/
MIT License
27 stars 16 forks source link

Attempting to show a twitter link as embedded fails #185

Closed TroZ closed 4 years ago

TroZ commented 4 years ago

Clicking the 'Show embedded media' for a twitter link results in nothing happening. This is for Version 1.70.

For example, clicking the 'Show embedded media' double arrow for the twitter link at https://www.shacknews.com/chatty?id=39987552#item_39987552 results in nothing happening.

Inspecting the page in Chrome show the following error: userscript.html?name=Shack-chan2.user.js&id=dd48132a-29a6-44f1-ba7e-e23bc6728c8d:1650 Uncaught TypeError: Cannot read property 'indexOf' of null at HTMLBodyElement.ClickListener (userscript.html?name=Shack-chan2.user.js&id=dd48132a-29a6-44f1-ba7e-e23bc6728c8d:1650)

This is the following line: if(url.indexOf("return clickItem")==0){

I have all Multimedia Embedding settings turned on except for 'Show Twitter threads when opening Twitter links.' Full settings: { "enabled_scripts": [ "shrink_user_icons", "reduced_color_user_icons", "media_loader", "social_loader", "getpost", "twitchauto", "nws_incognito", "dinogegtik", "sparkly_comic", "switchers", "new_comment_highlighter", "highlight_users", "custom_user_filters", "post_preview" ], "enabled_suboptions": [], "notificationuid": "", "user_filters": [], "username": "TroZ", "version": 1.7, "highlight_groups": [ { "built_in": false, "css": "border: 1px dotted white !important;", "enabled": true, "name": "Friends", "users": [] } ] }

WombatFromHell commented 4 years ago

I can't reproduce this on a clean browser profile with Chrome Shack 1.70 installed using your imported settings. The error you're getting isn't even from Chrome Shack. Double check your browser addons and reconfirm it's still happening.

TroZ commented 4 years ago

I've disabled other extensions and I'm still seeing this issue. I can send you screenshots of the console or do a screen share if that would help diagnose the issue.

On Sat, Sep 26, 2020 at 12:59 PM WombatFromHell notifications@github.com wrote:

I can't reproduce this on a clean browser profile with Chrome Shack 1.70 installed using your imported settings. The error you're getting isn't even from Chrome Shack. Double check your browser addons and reconfirm it's still happening.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/latestchatty/chromeshack/issues/185#issuecomment-699520758, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22I6JK7UQYFTVOXNUY3LSHYMYTANCNFSM4R276TPA .

WombatFromHell commented 4 years ago

I have no idea where that error is coming from, but it isn't coming from Chrome Shack or the Chatty page code (AFAIK). I'm not sure what to recommend to you to help you figure out what's actually causing it.

TroZ commented 4 years ago

Actually, I think you are somewhat correct - there was some interaction with another user script I had installed.

With tamper monkey disabled, I am no longer getting an exception, but the media is not embedding either.

On Sat, Sep 26, 2020 at 1:03 PM Brian Risinger brian.risinger@gmail.com wrote:

I've disabled other extensions and I'm still seeing this issue. I can send you screenshots of the console or do a screen share if that would help diagnose the issue.

On Sat, Sep 26, 2020 at 12:59 PM WombatFromHell notifications@github.com wrote:

I can't reproduce this on a clean browser profile with Chrome Shack 1.70 installed using your imported settings. The error you're getting isn't even from Chrome Shack. Double check your browser addons and reconfirm it's still happening.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/latestchatty/chromeshack/issues/185#issuecomment-699520758, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22I6JK7UQYFTVOXNUY3LSHYMYTANCNFSM4R276TPA .

WombatFromHell commented 4 years ago

There are a variety of reasons why the Twitter link might not be able to embed properly that have nothing to do with Chrome Shack. One possibility that comes to mind is that Twitter can block viewing a link by the country the request is originating from. I don't account for that in my frontend code, but I should probably put it on my todo list.

Can you verify that you can view the Twitter link via normal means?

TroZ commented 4 years ago

Yes, clicking the 'Open in new tab' button for the twitter link does work correctly for me.

I'm not sure how to debug this, as I don't see code from Chrome Shack in the inspector.

On Sat, Sep 26, 2020 at 1:20 PM WombatFromHell notifications@github.com wrote:

There are a variety of reasons why the Twitter link might not be able to embed properly that have nothing to do with Chrome Shack. One possibility that comes to mind is that Twitter can block viewing a link by the country the request is originating from. I don't account for that in my frontend code, but I should probably put it on my todo list.

Can you verify that you can view the Twitter link via normal means?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/latestchatty/chromeshack/issues/185#issuecomment-699523359, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22I2EOOTXP2EDMRIB22TSHYPFZANCNFSM4R276TPA .

TroZ commented 4 years ago

I'm noticing now that it isn't just twitter links, it seems to be all links that can be embedded, the embedding does not work.

On Sat, Sep 26, 2020 at 1:23 PM Brian Risinger brian.risinger@gmail.com wrote:

Yes, clicking the 'Open in new tab' button for the twitter link does work correctly for me.

I'm not sure how to debug this, as I don't see code from Chrome Shack in the inspector.

On Sat, Sep 26, 2020 at 1:20 PM WombatFromHell notifications@github.com wrote:

There are a variety of reasons why the Twitter link might not be able to embed properly that have nothing to do with Chrome Shack. One possibility that comes to mind is that Twitter can block viewing a link by the country the request is originating from. I don't account for that in my frontend code, but I should probably put it on my todo list.

Can you verify that you can view the Twitter link via normal means?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/latestchatty/chromeshack/issues/185#issuecomment-699523359, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22I2EOOTXP2EDMRIB22TSHYPFZANCNFSM4R276TPA .

WombatFromHell commented 4 years ago

Not sure what to tell you. It isn't Chrome Shack related. If you still think it might be you're welcome to poke around in a debug build (attached to this post) and let me know if you can narrow down an error that might be causing it, because I'm not able to reproduce what you're seeing with a clean browser profile with only uBlock Origin and Chrome Shack installed.

(edit: FYI, Instagram and Twitter endpoint traffic can be seen when first toggling an embed by going into the background page for Chrome Shack from the Extensions panel and looking at the Network tab of devtools. You might have to enable Developer mode in the Extensions panel to see that option though.)

TroZ commented 4 years ago

Thanks for your attempted help.

I barely understood how Chrome Shack worked a year ago, and now the code appears to be completely different. I can see that there is a media_loader and scoial_loader module, but I don't see where they are defined, when looking at the code of the unpacked extension in Chrome's inspector.

Loading the unpacked extension causes an exception every 2 seconds: WebSocket connection to 'ws://localhost:9090/' failed: Error in connection establishment: net::ERR_CONNECTION_REFUSED (anonymous) @ background.js:1305 background.js:1231 [ WER: Error trying to re-connect. Reattempting in 2s ] logger @ background.js:1231 ws.onerror @ background.js:1308 background.js:1231 [ WER: Attempting to reconnect (tip: Check if Webpack is running) ]

This makes it hard to try to debug as the console is just filled with these messages.

Looking at the code in the zip file, I am seeing a few methods such as detectMediaLink that it would be interesting to step through, however, in the inspector, I don't see content;js at all: [image: image.png]

The structure of the code is completely different than it was a year ago. There were separate files for each module that I'm no longer seeing.

My current guess as to the issue is that looking at the double arrow embedder button, I don't see any listeners other than the listeners that are on basically every element. However, I'm not sure if that is because of some issue adding the listener, or if you just don't see listeners of extensions in the inspector. [image: image.png] (This is with the unpacked extension loaded)

Thanks for looking into this. I don't expect you to look further into this as this apparently is only occurring on my machine.

On Sat, Sep 26, 2020 at 1:32 PM WombatFromHell notifications@github.com wrote:

Not sure what to tell you. It isn't Chrome Shack related. If you still think it might be you're welcome to poke around in a debug build (attached to this post) and let me know if you can narrow down an error that might be causing it, because I'm not able to reproduce what you're seeing with a clean browser profile with only uBlock Origin and Chrome Shack installed.

chromeshack-1.70-dev.zip https://github.com/latestchatty/chromeshack/files/5287091/chromeshack-1.70-dev.zip

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/latestchatty/chromeshack/issues/185#issuecomment-699524780, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22IYFOA7NGLR3SW26VJLSHYQSXANCNFSM4R276TPA .

WombatFromHell commented 4 years ago

The media embedding system is composed of a few parts. Links found in div.postbody go through the <MediaEmbedderWrapper /> React component (found in ./src/optional/media-embedder/) which get rendered as the <Expando /> React component. Most of the heavy lifting of resolving media and returning actionable components for rendering is done by a set of custom React hooks in ./src/core/useResolvedLinks/. I would start there if you're looking for potential culprits.

I realize that at first glance it seems like a complicated system, but it enables huge improvements to code maintainability, extensibility, and testability over the old way of directly manipulating the DOM. This way, adding a new embed type is as simple as implementing an API file with a few basic callbacks, then adding the parser callback to the master parser list in detectMediaLink() (found in ./src/core/api/index.ts) rather than touching the frontend rendering code.

(edit: Also, the console spam is due to the auto-reloader plugin in WebPack freaking out that there's no server telling it when to reload itself if files change. You can just ignore those messages directly in the Chrome console.)

TroZ commented 4 years ago

Thanks for the advice.

It took me a while to find content.js in Chrome's inspector. I found and put breakpoints in useResolvedLinks, and the other methods from /src/core/useResolvedLinks/index.tsx . I do see useResolvedLinks being called, but I don't see any of there other methods in that file being called. I'm not sure how that code works, so I don't know what is going on.

I have my own media embedder user script (that is what was throwing the exception I saw initially). I'll switch back to that for the time being. Perhaps the next update will fix whatever issue is preventing Chrome Shack's media embedding from working on my machine will get corrected then.

On Sat, Sep 26, 2020 at 3:54 PM WombatFromHell notifications@github.com wrote:

The media embedding system is composed of a few parts. Links found in div.postbody go through the React component (found in ./src/optional/media-embedder/) which get rendered as the <Expando /> React component. Most of the heavy lifting of resolving media and returning actionable components for rendering is done by a set of custom React hooks in ./src/core/useResolvedLinks/. I would start there if you're looking for potential culprits.

I realize that at first glance it seems like a complicated system, but it enables huge improvements to code maintainability, extensibility, and testability over the old way of directly manipulating the DOM. This way, adding a new embed type is as simple as implementing an API file with a few basic callbacks rather than touching the frontend rendering code.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/latestchatty/chromeshack/issues/185#issuecomment-699540673, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22IZWUZETRIHUSEGLSALSHZBG3ANCNFSM4R276TPA .

WombatFromHell commented 4 years ago

You said you set breakpoints in the useResolvedLinks() hook; did you see if resolveChildren({...}) was returning anything? If not, are you actually getting any network traffic from the Twitter API when toggling a link for the first time?

The only thing I can think to do programmatically is to maybe add a reload button of sorts in case a link doesn't load properly when initially being toggled (like Chattypics links have a tendency to do). Right now, as you can see in the hook, the returned component (which can also be an empty div) gets cached and from that point on <Expando /> will only return that cached component until the post is reloaded and the link re-embedded.

I'll be adding more user-facing options for managing the behavior of the media embedder in the next version so hopefully that might solve some of your issue.

TroZ commented 4 years ago

If you are talking about the resolveChildren method just above useResolvedLinks, I was not seeing it get called at all.

On Sat, Sep 26, 2020 at 5:59 PM WombatFromHell notifications@github.com wrote:

You said you set breakpoints in the useResolvedLinks() hook; did you see if resolveChildren({...}) was returning anything? If not, are you actually getting any network traffic from the Twitter API when toggling a link for the first time?

The only thing I can think to do programmatically is to maybe add a reload button of sorts in case a link doesn't load properly when initially being toggled (like Chattypics links have a tendency to do). Right now, as you can see in the hook, the returned component (which can also be an empty div) gets cached and from that point on will only return that cached component until the post is reloaded and the link re-embedded.

I'll be adding more user-facing options for managing the behavior of the media embedder in the next version so hopefully that might solve some of your issue.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/latestchatty/chromeshack/issues/185#issuecomment-699553238, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22I6SPDVCCUVTUQ4RCQTSHZP4FANCNFSM4R276TPA .

WombatFromHell commented 4 years ago

It still sounds to me like something else that's running on your browser profile is preventing Chrome Shack's media embedder from working properly. It's worth noting that Chrome Shack's media embedder requires that the original anchor link be removed from the DOM so that a React component can be rendered in its place. Anything that mucks with anchor links on the page will probably break Chrome Shack's embedder or vice versa.

WombatFromHell commented 4 years ago

I've done some refactoring and added a reload button to <Expando />, and I'd like to know if this build might have fixed or at least changed the behavior of this issue. Let me know if you get a chance to test it. Be aware that this build is a WIP from an upcoming release so it may contain other bugs.

TroZ commented 4 years ago

Hi,

Sorry, I was busy this weekend and didn't notice your emails.

Test2 does show the content after I click the reload button. However, even after clicking the reload button, the double arrow expando button does not function to collapse the content again. Overall, it's a big improvement as it does allow the content to be seen and interacted with, however it isn't completely working on my machine.

On Sun, Sep 27, 2020 at 6:21 PM WombatFromHell notifications@github.com wrote:

I've done some refactoring and added a reload button to , and I'd like to know if this build might have fixed or at least changed the behavior of this issue. Let me know if you get a chance to test it. Be aware that this build is a WIP from an upcoming release so it may contain other bugs.

chrome_shack-1.71-i185-test2.zip https://github.com/latestchatty/chromeshack/files/5289180/chrome_shack-1.71-i185-test2.zip

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/latestchatty/chromeshack/issues/185#issuecomment-699695404, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22IZFYWRMGGISMC423PDSH63E3ANCNFSM4R276TPA .

WombatFromHell commented 4 years ago

Hmm, okay, I've done some more simplification and refactoring. Try this build and let me know if it improves things even more. I may have found what was causing the problem, if so.

TroZ commented 4 years ago

Hmm, this version doesn't seem as good as test 2 to me.

The double arrow embed button still doesn't work initially. The reload button does animate when clicked, and I can see the legacy and mercury icons after everyone's names brighten and then dim again (as I have the option to dim them turned on). However, the content is not embedded after clicking reload, which was happening in test 2. The embed double arrow button still does not work after clicking reload, nothing happens when it is clicked.

If you want, I can try to do a screenshare of some sort over discord od something so you can direct me to put some breakpoints to see what is going on.

On Mon, Sep 28, 2020 at 1:33 PM WombatFromHell notifications@github.com wrote:

Hmm, okay, I've done some more simplification and refactoring. Try this build and let me know if it improves things even more. I may have found what was causing the problem, if so.

chrome_shack-1.71-i185-test3.zip https://github.com/latestchatty/chromeshack/files/5294033/chrome_shack-1.71-i185-test3.zip

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/latestchatty/chromeshack/issues/185#issuecomment-700177875, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22IYVV3G4UGGUGC4NKWTSIDCGDANCNFSM4R276TPA .

WombatFromHell commented 4 years ago

Okay, if this next build doesn't work out you can DM me on the Shack Discord and I'll help with debugging:

chrome_shack-1.71-i185-test6-dev.zip

TroZ commented 4 years ago

I don't know what you changed, but it seems to be working perfectly now!

Thanks for looking into this issue!

On Mon, Sep 28, 2020 at 8:42 PM WombatFromHell notifications@github.com wrote:

Okay, if this next build doesn't work out you can DM me on the Shack Discord and I'll help with debugging:

chrome_shack-1.71-i185-test6-dev.zip https://github.com/latestchatty/chromeshack/files/5295726/chrome_shack-1.71-i185-test6-dev.zip

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/latestchatty/chromeshack/issues/185#issuecomment-700357401, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22I5SI4MQHEH4ZBKVQSTSIEUQJANCNFSM4R276TPA .

WombatFromHell commented 4 years ago

Thanks for helping me get to the bottom of it. I'm sure other users will also appreciate it. I'll make sure to credit you in the release notes for bug testing.

WombatFromHell commented 4 years ago

Should be fixed as of 6f68d4364010417a775b048f9282dd66c30753ce