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

Issue #213 - UI - Initial support for ActivityPub comments #218

Conversation

dellagustin
Copy link
Contributor

@dellagustin dellagustin commented Jan 28, 2023

This commit introduces initial support for showing comments. It relies on the API also including socialInteracts and on a new API on the server being implemented to fetch the comments.

This is not yet ready, I have implemented some mock code on the server side that is not included in this commit.

The front-end code expects the comments API to respond with the same format as podverse's /api/v1/episode/{episodeid}/proxy/activity-pub, which is exactly what is returned by threadcap.

To Do

Limitations

Performance

When this PR is ready for merge, it will still be limited to fetching all comments in a single shot, and the UI will wait until they are all fetched, which can take some time.

A potential improvement would be some kind of "long pooling", where the server sends partial updates back each time a new comment is fetched. This way the UI could be progressively updated. Maybe this can be achieved with https://developer.mozilla.org/en-US/docs/Web/API/Streams_API/Using_readable_streams.

I have created an issue to track this future improvement: #228

Summary / Content warning

The property summary is not taken into account, which is returned by Pleroma and Mastodon posts as a Content warning (e.g. to avoid spoilers). This is, to my best knowledge, also a limitation of threadcap (see skymethod/minipub#6), not anymore, see #218 (comment). I set this now as a stretch goal, could be easily added later in a compatible way with a separate PR, if this one is dragging too long.

Attachments

Attachments are currently not shown (e.g. attached images).

I have created an issue to track this future improvement: #229.

Scaling

As all the API calls are going out from the same IP, if many users are checking comments in parallel, they could reach the instances' rate limit. For instance, the default for Mastodon instances is 300 calls within 5 mins (mastodon rate limits).

Related issues

Dependencies

Links and References

This commit introduces initial support for showing comments.
It relies on the API also including socialInteracts and on a
new API on the server being implemented to fetch the comments.

This is not yet ready, I have implemented some mock code on
the server side that is not included in this commit.

The front-end code expects the comments API to respond with
the same format as podverse's
/api/v1/episode/{episodeid}/proxy/activity-pub

Related issues:
- resolves Podcastindex-org#213
- Podcastindex-org#213
@dellagustin
Copy link
Contributor Author

dellagustin commented Jan 28, 2023

Additional server side code I'm using at server/index.js, to mock the API data:

const fakeCommentsResponse = require('./fake-comments-response.json')
app.use('/api/episodes/byfeedid', async (req, res) => {
    let feedId = req.query.id
    let max = req.query.max
    const response = await api.episodesByFeedId(feedId, null, max)

    // Injects fake socialInteract content here for testing
    response.items.forEach((item) => {
        item.socialInteracts = [{
            uri: 'https://podcastindex.social/users/dave/statuses/109683341113064081',
            protocol: 'activitypub',
        }]
    })

    res.send(response)
})
// ------------------------------------------------
// ------------ API to get comments for episode ---
// ------------------------------------------------
app.use('/api/comments/byepisodeid', async (req, res) => {
    let feedUrl = req.query.url
    // fakeCommentsResponse was taken directly from the API used by podverse
    res.send(fakeCommentsResponse)
})

For server/fake-comments-response.json I simply inspected the return of the API in podverse and copied the response body.

Current result:
image

@dellagustin
Copy link
Contributor Author

@daveajones, we'll need to sanitize the content of the comments, do you have any library recommendations?

@daveajones
Copy link
Contributor

Threadcap doesn't sanitize?

This commit introduces the functionality of fetching comments
on the server using threadcap through the API
`/api/comments/byepisodeid?id={episode id}`.

it currently fetches as comments and replies, and wait untill
all comments are fetched. It takes some time (circa 7s during
my tests).

With this commit two new dependencies were introduced.

Additionally, error handling was added for possible fetch
issues for comments and replies (which were detected
during tests)
@dellagustin
Copy link
Contributor Author

dellagustin commented Jan 29, 2023

Threadcap doesn't sanitize?

@daveajones I'm not sure, but I am assuming it doesn't. I could not find information indicating whether it does or does not on the documentation. I asked @johnspurlock-skymethod, see skymethod/minipub#5 and https://fosstodon.org/@dellagustin/109771551290816322.

Now that's the server is doing requests to 3rd party servers, we should define a user agent. Do you have any preferences on this one?

I'll move forward with missing pieces, but soon it will be blocked only by Podcastindex-org/docs-api#91.

Could you do a "call to action" for the community to contribute on the "beautification" part? Maybe on the next Podcasting 2.0 episode? This is a part were I'm very weak.

@dellagustin
Copy link
Contributor Author

Current error handling:

image

image

@daveajones
Copy link
Contributor

I will 100% do a call out on the next board meeting. For user agent I typically use Podcastindex.org/v1.x so maybe something like:

Podcastindex.org-web/v1.0 ?

@Clay-Ferguson
Copy link

@daveajones, we'll need to sanitize the content of the comments, do you have any library recommendations?

I recommend DOMPurify NPM module for sanitizing. I have been using it in my project "Quanta" for a while.

@dellagustin
Copy link
Contributor Author

One current limitation, I'm only taking into account English language content.

threadcap returns the content of the comments as language-tagged strings:

"content": {
          "en": "content in English"
        }

I'm currently hardcoding to English.

Note that the content on the Activity is not always language-tagged, and when it is, this is under the property contentMap instead of content.

e.g.:

"content": "content in English",
    "contentMap": {
        "en": "content in English"
    },

I was remembered about this when testing with a reply from a different account where it is not set to English (simplified example below):

{
    "@context": [
        "https://www.w3.org/ns/activitystreams",
        "https://blob.cat/schemas/litepub-0.1.jsonld",
        {
            "@language": "und"
        }
    ],
    "content": "Content in und",
}

Note that in the absence of a contentMap threadcap seems to be building one from content and @language.

@dellagustin dellagustin changed the title Issue #213 - UI - Initial support for comments Issue #213 - UI - Initial support for ActivityPub comments Jan 30, 2023
@dellagustin
Copy link
Contributor Author

@daveajones

Podcastindex.org-web/v1.0 ?

Sounds good to me. Is it ok if I read the version from package.json to build the final user agent?

@dellagustin
Copy link
Contributor Author

@nathangathright
Copy link
Contributor

CleanShot 2023-01-31 at 09 07 54

I’ve just updated the CodePen with a custom disclosure chevron, better support for Safari, and some space-saving mobile enhancements.

@dellagustin
Copy link
Contributor Author

dellagustin commented Jan 31, 2023

@nathangathright my mouse pointer is going a bit haywire on the clickable elements of the UI at the current version, but it may be a codepen thing, let's see if it happens when I implement it in PI.
It shows a hand cursor as if it was clickable, but if I let it there it goest back to being an arrow.
I'll probably leave the <relative-time> usage out of this PR as it adds another dependency, if I understood it correctly, and implement it afterwards with a dedicated PR.
Is this this the one you used, https://github.com/github/relative-time-element?

@dellagustin
Copy link
Contributor Author

dellagustin commented Jan 31, 2023

So an expectation management message: as you can see, the To Do list is packed. What I did so far was during a "time multiplexing window" that I dedicate to this project, now I have to do the same to other projects 😄 . Also, I have the feeling we are at a "80% of the functionality with 20% of the effor" point. In other words, what is left may take more time than you would expect to be ready. I'm guessing something like 4 weeks with me "multiplexing" between projects and life. Let's see.

@johnspurlock-skymethod
Copy link

The property summary is not taken into account, which is returned by Pleroma and Mastodon posts as a Content warning (e.g. to avoid spoilers). This is, to my best knowledge, also a limitation of threadcap (see skymethod/minipub#6).

Just for you Guilherme, this morning I added summary to the comment payload in the latest version of Threadcap - version 0.1.10

@nathangathright
Copy link
Contributor

@dellagustin Yeah, cursor:pointer on absolutely-positioned pseudo-elements might not be the best idea. Given that all interactive elements don't need a hand cursor, I’ll remove that affordance.

I’ve pushed an updated code to a stand-alone repo with GitHub Pages so you can see the behavior without CodePen’s interference. Additionally, I’ve mocked up what Summarized/Content Warnings could look like.

https://nathangathright.github.io/comment-thread-mockup/

CleanShot 2023-01-31 at 16 43 16

@nathangathright
Copy link
Contributor

nathangathright commented Jan 31, 2023

I'll probably leave the <relative-time> usage out of this PR as it adds another dependency, if I understood it correctly, and implement it afterwards with a dedicated PR. Is this this the one you used, https://github.com/github/relative-time-element?

Yeah, that’s fine. Just replace that custom element with the HTML5 standard <time> element. And yes, I just used unpkg to include GitHub’s package as a module at the bottom of the page, and everything else took care of itself.

@dellagustin
Copy link
Contributor Author

@johnspurlock-skymethod , @nathangathright you both have to slow down on your great and timely contributions, it is making me look bad 😛.
Seriously now, the teamwork here is amazing, open source Christmas came early this year for me. I cannot wait to get this finished and integrated, it will be awesome!

@dellagustin
Copy link
Contributor Author

@nathangathright , one note about the Content Warning, event though that the semantic for summary used by (at least) Mastodon and Pleroma, they show this in a very "neutral" way, meaning: they call it Content Warning when your are editing, but they don't refer to it when they are showing it, they just hide the content under a Show More/Show Content user interaction. Screenshots below.
I like this neutral approach in the context of ActivityPub, as the Activity Vocabulary does not define a way to represent content warnings or spoiler alerts (that I could find), so summary is intended to be just a summary, and the neutra approach does not go against that.
In summary (😜), I suggest our implementation support summary in a similar way, just show the summary and have a Show more or Show content button.

image
image

image
image

@dellagustin
Copy link
Contributor Author

Testing with threadcap v0.1.10 showed a regression, reported with skymethod/minipub#7.

This commit adds support for the `summary` property, which
is normally used also as _content warning_.
@dellagustin
Copy link
Contributor Author

@nathangathright , I just implemented the support for summary. When I saw that your css trickery saved me from managing the state of Show More / Less, I was relieved!

This commit solves a react warning due to missing key on
list elements. It uses the comment url as key, as there
it no expectation that the same comment will appear twice in
the same replies list.
@dellagustin dellagustin force-pushed the enhancement#213__show-episode-comments branch from f021d51 to 22f868b Compare February 5, 2023 22:39
@dellagustin
Copy link
Contributor Author

I have been thinking about how stream back the comments processed by threadcap to the front-end, so that the user does not need to wait for the threadcap to finish its work to start seeing comments.

The idea is to use the threadcap callback and send back a JSON with new nodes and and commenters each time there is a node-processed event for a comment, then rebuilt the response on the frontend based on these chunked updates and update the state each time a chunk arrives.

@dellagustin
Copy link
Contributor Author

Indication that comments are loading is there:
image
Ugly, but it works 😄
@nathangathright , if you feel like beautifying that, come along.
Also, did you see my overflow message from earlier - #218 (comment)?
One more TODO to go and the blockers from my side would be gone - I'm racing @daveajones here, hoping to finish this before he finishes introducing socialInteract to the API 😜.

@nathangathright
Copy link
Contributor

@dellagustin The CSS for the summary element didn't account for display names + handles + long timestamps. Adding width:100%; overflow:hidden; on that element would patch the problem.

But I’ve gone a bit further and pushed an update to my little repo with stacked name and handles on desktop and hiding the handles on mobile.

(I still think handles are unnecessary given that you can click through to the original profile to verify anyone’s identity.)

@dellagustin
Copy link
Contributor Author

Hello @nathangathright , thank you for the changes. I can see they work well in your standalone mock up, but when I implemented them inside the podcast index, it still overflows, here is a screenshot:
image

@dellagustin
Copy link
Contributor Author

Fixed it by adding width: 100% to an outer div I added with the comments section.
I did remove the body CSS that came with @nathangathright 's mock up, as I feared it would affect the rest of the site somehow.

Temp code was added by mistake in
2f826a7
As Podcastindex-org/docs-api#91 is
now implemented, this commit replaces the hard coded
comment uri by the content of the socialInteract property
from the podcast index API.

Note: even though the spec does not forbid two socialInteracts with
the same protocol, this implementation only takes the first one. If
we see use cases for supporting both, then we will need to update
later.
This commits add sanitization using for comments summary and
content using DOMPurify.

To test it, I faked the response of `/api/comments/byepisodeid`
with a hard coded json file containing in the content of comments
some examples from the section "Some purification samples please?".

References:
- https://www.npmjs.com/package/dompurify
Updated yarn.lock.
There are a lot of changes, but the versions there
do not look like they were drastically changed.
I'm assuming some changes are related to using a newer
yarn version, in my case, 3.4.1.
@dellagustin
Copy link
Contributor Author

Content sanitization added with 07207d2 using DOMPurify.

To test it, I faked the response of /api/comments/byepisodeid
with a hard coded json file containing in the content of comments
some examples from the section "Some purification samples please?".

References:

@dellagustin dellagustin marked this pull request as ready for review February 11, 2023 16:11
@dellagustin
Copy link
Contributor Author

dellagustin commented Feb 11, 2023

@daveajones , ready for review 🎉 .
special thanks to @johnspurlock-skymethod and @nathangathright for the help with threadcap and the design and code for the frontend.

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

Successfully merging this pull request may close these issues.

Add support for comments
6 participants