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.

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

feat: add p2p URL support across all content in the reader #20

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

akhileshthite
Copy link
Collaborator

closes #17

Facing two issues:

  1. Getting null src for img and video in the post content. The src attributes might be empty or not set at the time when the script tries to update them?
    const images = contentDOM.querySelectorAll('img')
    images.forEach(img => {
      const src = img.getAttribute('src')
      console.log('Original src:', src)
      img.setAttribute('src', resolveP2PUrl(src))
    })

    const videos = contentDOM.querySelectorAll('video source')
    videos.forEach(video => {
      const src = video.getAttribute('src')
      video.setAttribute('src', resolveP2PUrl(src))
    })
  1. With the same updates in random sort timelines branch, getting this error:
    Screenshot 2024-05-26 at 3 13 29 AM

@akhileshthite akhileshthite self-assigned this May 25, 2024
@akhileshthite akhileshthite added the enhancement New feature or request label May 27, 2024
Copy link
Contributor

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

We shouldn't be resolving to gateways in browsers that support p2p urls. Instead see if there's a DOM event that gets emitted when an image/video fails to load and listen to it at the document.body then take the event.target and attempt to rewrite its src. Video takes also have different ways of specifying sets of sources so we should account for that too.

@RangerMauve
Copy link
Contributor

For the searchNotes bit, check that you fixed the issues in #13 where the function got accidentally deleted

@akhileshthite
Copy link
Collaborator Author

We shouldn't be resolving to gateways in browsers that support p2p urls. Instead see if there's a DOM event that gets emitted when an image/video fails to load and listen to it at the document.body then take the event.target and attempt to rewrite its src. Video takes also have different ways of specifying sets of sources so we should account for that too.

Yep, will add the fallback logic.

This approach is not working:

    // Setup error listeners for media elements within this component
    this.addEventListener('error', event => {
      const target = event.target;
      if (target.tagName === 'IMG' || (target.tagName === 'SOURCE' && target.parentElement.tagName === 'VIDEO')) {
        const originalSrc = target.getAttribute('src');
        if (!originalSrc.startsWith('hyper://') && !originalSrc.startsWith('ipns://')) {
          target.setAttribute('src', resolveP2PUrl(originalSrc));
        }
      }
    }, true);
  }

@RangerMauve
Copy link
Contributor

What part doesn't work? What steps have you taken to debug it so far?

@akhileshthite
Copy link
Collaborator Author

akhileshthite commented Jun 5, 2024

What part doesn't work? What steps have you taken to debug it so far?

I experimented with load events just to verify the functionality and discovered that IPNS and HYPER URLs are resolving as null, including the original/initial URLs. It may be due to the specific timing of how we handle p2p URLs, particularly when elements are processed before they are fully loaded or accessible in the DOM?

Example code:-

  connectedCallback() {
      // Listen for load events on the entire document body for images and video sources
      document.body.addEventListener('load', event => {
        const target = event.target;
    
        // Check if the loaded element is an image or a video source
        if (target.tagName === 'IMG' || (target.tagName === 'SOURCE' && target.closest('video'))) {
          const originalSrc = target.getAttribute('src');
    
          if (originalSrc && !originalSrc.startsWith('hyper://') && !originalSrc.startsWith('ipns://')) {
            const newSrc = resolveP2PUrl(originalSrc);
            console.log('Rewriting src for:', target, 'from:', originalSrc, 'to:', newSrc);
            target.setAttribute('src', newSrc);
          }
        }
      }, true);
  }

@RangerMauve
Copy link
Contributor

By resolving as null do you mean the originalSrc attribute is showing up as null or is the resolveP2PURL function returning null?

@RangerMauve
Copy link
Contributor

if (!originalSrc.startsWith('hyper://') && !originalSrc.startsWith('ipns://')) {
          target.setAttribute('src', resolveP2PUrl(originalSrc));
        }

This seems to only be trying to resolve the URL if it isn't p2p. Should the if statement be resolved and have us try to update the src attribute right away?

Which browser are you testing this on? Which URL/post?

@RangerMauve
Copy link
Contributor

Mind committing your code?

@akhileshthite
Copy link
Collaborator Author

Mind committing your code?

Fixed it!
Will commit in a few hours.

@akhileshthite
Copy link
Collaborator Author

Mind committing your code?

Fixed it! Will commit in a few hours.

Resolving urls before rendering in loadAndRenderPost worked: 675329f

Regarding the gateway fallback how is this approach in db.js?

export async function supportsP2P (url) {
  console.log(url)
  try {
    const response = await fetch(url)
    return response.ok
  } catch (error) {
    console.log('P2P URL loading failed:', error)
    return false
  }
}

export async function resolveP2PUrl (url) {
  if (!url) return url

  console.log('Attempting to resolve P2P URL:', url)

  if (isP2P(url)) {
    const p2pSupported = await supportsP2P(url)

    if (!p2pSupported) {
      if (url.startsWith(HYPER_PREFIX)) {
        return url.replace(HYPER_PREFIX, 'https://hyper.hypha.coop/hyper/')
      } else if (url.startsWith(IPNS_PREFIX)) {
        return url.replace(IPNS_PREFIX, 'https://ipfs.hypha.coop/ipns/')
      }
    }
  }

  return url
}

@RangerMauve
Copy link
Contributor

Can you elaborate on what the purpose of the proposed changes to db.js are?

@@ -50,7 +51,7 @@ class ActorMiniProfile extends HTMLElement {
// Actor icon
const img = document.createElement('img')
img.className = 'profile-mini-icon'
img.src = iconUrl
img.src = resolveP2PUrl(iconUrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't resolve to http unless there's an error loading. Maybe we should have a p2p-image custom element made which will attempt to render an img and fall back with the onerror handler? Then we can reuse the logic across pages.

db.js Outdated
@@ -103,6 +115,8 @@ export class ActivityPubDB extends EventTarget {
if (url && typeof url === 'object') {
return url
}
url = resolveP2PUrl(url)
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be trying to resolve to the gateway right away. Keep the current logic which resolve to the gateway only when the p2p logic fails.

post.js Outdated
console.log(`Set video src to: ${p2pSrc}`)
})

return contentDOM.body.innerHTML // Return the modified HTML to be inserted
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the onerror events actually fire here? I think we need to add these listeners after we've sanitized and rendered the html for the post.

@akhileshthite
Copy link
Collaborator Author

Can you elaborate on what the purpose of the proposed changes to db.js are?

If the fetch fails, it will resolve the p2p URLs using gateways.

@akhileshthite
Copy link
Collaborator Author

58366b8

The p2p-media.js is resolving to gateway if the fetch fails:
Screenshot 2024-06-13 at 7 41 12 PM

The problem is it's not replacing the src:
original img src:
Screenshot 2024-06-13 at 7 41 57 PM

p2p-image src:
Screenshot 2024-06-13 at 7 42 09 PM

Copy link
Contributor

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

We should modify the supportsP2P function so it only checks for the p2p schemes once.

something like this

  • have a global map of <protocol promise<boolean> | boolean>
  • supports will get the scheme from the src
  • check if the map has a value for the scheme if(supportMap.has(scheme)) return await supportMap.get(scheme)
  • if doesn't have, set a promise for attempting to resolve the url scheme
  • if request fails on a network error (no http response) set it to false and return false
  • else set to true

This will reduce the number of http requests and improve performance.

p2p-media.js Outdated
}

handleError (source) {
const fallbackSrc = resolveP2PUrl(source.src)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, check if the current img.src is p2p

profile.html Outdated
@@ -40,6 +40,7 @@
<script type="module" src="./sidebar.js"></script>
<script type="module" src="./actor-profile.js"></script>
<script type="module" src="./post.js"></script>
<script type="module" src="./p2p-media.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have post.js import any of it's dependencies directly instead of doing it top level.

Similarly outbox should import post.

@RangerMauve
Copy link
Contributor

We already have a fallback for detecting when p2p fetch fails: https://github.com/hyphacoop/reader.distributed.press/blob/main/db.js#L116 Lets stick to that without adding new stuff

…ensure correct styling

feat: ensure class attributes are correctly synchronized for media elements

feat: optimize protocol support check with global caching

refactor: import module dependencies directly in respective scripts
@akhileshthite
Copy link
Collaborator Author

akhileshthite commented Jun 20, 2024

p2p-image and p2p-video are working fine everywhere; the profile pictures in actor-profile, actor-profile-mini, and followed-accounts.

The media is not loading in the post content, I think there's something wrong with insertImagesAndVideos:

post.js:55 Updated p2p-image src after fallback: https://hyper.hypha.coop/hyper/hypha.coop/assets/images/posts/2022-05-12-ppl-sitting-unsplash.jpg
post.js:59 Replaced img with p2p-image having initial src: hyper://hypha.coop/assets/images/posts/2022-05-12-ppl-sitting-unsplash.jpg

I'm currently debugging the issue.

@akhileshthite
Copy link
Collaborator Author

We should modify the supportsP2P function so it only checks for the p2p schemes once.

something like this

  • have a global map of <protocol promise<boolean> | boolean>
  • supports will get the scheme from the src
  • check if the map has a value for the scheme if(supportMap.has(scheme)) return await supportMap.get(scheme)
  • if doesn't have, set a promise for attempting to resolve the url scheme
  • if request fails on a network error (no http response) set it to false and return false
  • else set to true

This will reduce the number of http requests and improve performance.

export async function supportsP2P (url) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Loading of p2p URLs for All Content
2 participants