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

Mutual Checker, NotificationBlock, Quote Replies: Process new reply format #1659

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

marcustyphoon
Copy link
Collaborator

@marcustyphoon marcustyphoon commented Dec 5, 2024

Well, #1658 is fairly problematic.

Description

This contains (somewhat rudimentary) fixes for the linked issue, in which the relevant features are broken by certain kinds of activity item that use a new internal data format, containing only enough information to render the notification rather than all of the internal data we use to add functionality.

  • Mutual Checker can still easily determine if the replying user is a mutual, since Tumblr renders that.
  • NotificationBlock wants to know the root post of the target post, which just so happens to work in this case because the activity items in question are on original posts, but will be a problem in the future if all activity items switch to this API.
  • As per below discussion, with some slightly hairy URL and NPF parsing, Quote Replies can look up and fetch the correct reply and get the summary text from the notification data.
outdated Quote Replies info
  • Quote Replies... is a somewhat complicated scenario because we might technically have enough information to look up the relevant post and post activity using an API fetch, but only via very convoluted methods (the activity item links to the post; we could parse the url to get the relevant information to API fetch the post but we have no timestamp with which to easily find the reply). For now, just to make it work on all posts rather than keep getting confused users, I've processed the activity item itself into a rudimentary post body:

This truncates the reply text and generally looks rather stupid, but it's better than nothing. Note also that it does not currently exclude community posts (I didn't see a way to do so). I did test the code on a reply on a community post.

Testing steps

To easily distinguish affected activity items, run document.documentElement.append(Object.assign(document.createElement('style'), { textContent: '[aria-label="Notification"]:not([role="listitem"]) { outline: 1px solid red; outline-offset: -3px }' })).

Mutual Checker:

  • Enable the "only show notifications from mutuals" option.
  • Confirm that from-mutuals notifications of both the old and new types are visible.
  • Confirm that other notifications of both the old and new types are hidden.

NotificationBlock:

  • Find or generate a new-type notification about one of your posts; this will begin with "Replied to your post."
  • Activate NotificationBlock on the linked post and confirm that the notification is hidden.

Quote Replies:

  • Confirm that Quote Replies adds action buttons to "replied to your post", "replied to you in a post", and "mentioned you on a post" notifications. (I do this by enabling responsive design mode and emulating a mobile device, which will show the buttons without hovering them.)
  • Confirm that Quote Replies does not add action buttons to "replied to you in a [community name] post" or "mentioned you in a comment in [community name]" notifications.
  • Confirm that Quote Replies generates draft posts with the correct contents from all three supported notification types, from both the activity page and activity modal.
  • Break the Quote Replies API fetch. Confirm that Quote Replies generates draft posts from "replied to your post" / "replied to you in a post" notifications, with degraded formatting. Confirm that "mentioned you on a post" notifications will instead show an error modal (this code path is unchanged and we didn't write a fallback into it).
  • Confirm that the links in the generated draft posts all point to the correct accounts/urls.

@marcustyphoon marcustyphoon force-pushed the new-notification-fix branch 3 times, most recently from 5ae2b74 to 49ff16e Compare December 9, 2024 23:47
@marcustyphoon marcustyphoon changed the title partial new notification fix Mutual Checker, NotificationBlock, Quote Replies: Process new reply format Dec 9, 2024
@marcustyphoon marcustyphoon marked this pull request as ready for review December 10, 2024 00:02
transform: scale(0);
}

${notificationSelector}:is(:hover, :focus-within) button.xkit-quote-replies svg {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably should have done the switch to styleElement in two commits, now that I think about it. For reference, this should be the only changed line in the CSS.

@marcustyphoon
Copy link
Collaborator Author

marcustyphoon commented Jan 6, 2025

Hm... maybe this partial implementation of Quote Replies should include an informational modal each time it's used?

Re: Quote Replies, see also: #1662.

@AprilSylph AprilSylph self-assigned this Jan 19, 2025
@AprilSylph
Copy link
Owner

This truncates the reply text and generally looks rather stupid, but it's better than nothing. Note also that it does not currently exclude community posts (I didn't see a way to do so). I did test the code on a reply on a community post.

brain dump of my current thinking about how to fetch specific replies:

  1. blog identifier and post ID can be reliably parsed notification.actions.tap.href. if new URL(temp1.__reactFiber$i3lhlxz35zo.return.return.memoizedProps.notification.actions.tap.href).pathname.split('/')[1] === 'communities' then we know it's a communities URL, otherwise it's a blog URL. blog URLs need any leading @ stripped to be used as an API blog identifier, communities handles need @@ prepended to be used as an API blog identifier.
  2. fetch replies on the target post using the above and the notification.timestamp
  3. filter fetched replies to those with a matching timestamp
  4. pick the first fetched reply that has a partial match for the notification.body.content[1].text (with the trailing ellipsis omitted)
  5. if there is no such match (how can this happen? newlines? can we massage the fetched reply text in the same way as the tumblr API?), fall back to the notification indented text

also notification.avatar.badgeImage might be useful for splitting on different activity types at some point idk

@marcustyphoon
Copy link
Collaborator Author

marcustyphoon commented Feb 10, 2025

Wait, there is a timestamp... hm, why did I think there wasn't a timestamp? I have screenshots of the API data from when I was working on this and there absolutely is a timestamp that I must have missed. (Unless that's the post timestamp or something absurd, but why would that be the case).

Er, stay tuned.

@marcustyphoon marcustyphoon self-assigned this Feb 10, 2025
@marcustyphoon
Copy link
Collaborator Author

  1. blog identifier and post ID can be reliably parsed notification.actions.tap.href. if new URL(temp1.__reactFiber$i3lhlxz35zo.return.return.memoizedProps.notification.actions.tap.href).pathname.split('/')[1] === 'communities' then we know it's a communities URL, otherwise it's a blog URL. blog URLs need any leading @ stripped to be used as an API blog identifier, communities handles need @@ prepended to be used as an API blog identifier.

Done. (I assume there's no way for a t: blog UUID to wind up here, right?)

  1. fetch replies on the target post using the above and the notification.timestamp

  2. filter fetched replies to those with a matching timestamp

  3. pick the first fetched reply that has a partial match for the notification.body.content[1].text (with the trailing ellipsis omitted)

Done. Well, sort of; I call the existing code here, which assumes that the first reply with a matching timestamp is the right one. We should... probably add that partial match check?

  1. if there is no such match (how can this happen? newlines? can we massage the fetched reply text in the same way as the tumblr API?), fall back to the notification indented text

As implemented, this falls back on any failure of the processing code.

@marcustyphoon marcustyphoon removed their assignment Feb 10, 2025

const targetPostSummary = bodyDescriptionContent.text.slice(summaryFormatting.start + 1, summaryFormatting.end - 1);

return await processReply({ type, timestamp, targetPostId, targetTumblelogName, targetPostSummary });
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should this be two lines (const { content, tags } = await processReply(/* */); return { content, tags };)? I know some people instinctively recoil at "return await" but it's necessary here to trigger the synchronous catch block.

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.

Mutual Checker, NotificationBlock, Quote Replies: Some notifications are not processed
2 participants