Skip to content

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

Merged
marcustyphoon merged 10 commits intoAprilSylph:masterfrom
marcustyphoon:new-notification-fix
Apr 22, 2025
Merged

Mutual Checker, NotificationBlock, Quote Replies: Process new reply format#1659
marcustyphoon merged 10 commits intoAprilSylph:masterfrom
marcustyphoon:new-notification-fix

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
@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 and others added 2 commits February 10, 2025 06:25
Co-Authored-By: April Sylph <28949509+AprilSylph@users.noreply.github.com>
@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
@AprilSylph
Copy link
Owner

I'm gonna sit on this one this week too, but only because there's something cooking which should make reply fetching a lot more reliable...

@AprilSylph
Copy link
Owner

Important note for future reference: Mutual Checker and NotificationBlock could've been fixed months ago if their changes had been split into their own PRs.

Granted, all these updates rely on updated utils, but if you include the same util changes across multiple PRs, you open the possibility to first 1) get the util changes merged with the least effort from the reviewer (i.e. with the lowest-reviewer-effort feature update) and then 2) update all the PRs so that the util changes aren't included in feature updates, which further reduces reviewer effort.

Copy link
Owner

@AprilSylph AprilSylph left a comment

Choose a reason for hiding this comment

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

Quote Replies doesn't seem to work on "Replied to you in a post" notifications:

Screen Shot 2025-04-22 at 11 45 14


EDIT: Looks like this is only the case for posts with no text / notifications with no summary text.

Copy link
Owner

@AprilSylph AprilSylph left a comment

Choose a reason for hiding this comment

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

  • Mutual Checker
    • 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
    • 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.
    • 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
Copy link
Collaborator Author

Important note for future reference: Mutual Checker and NotificationBlock could've been fixed months ago if their changes had been split into their own PRs.

Granted, all these updates rely on updated utils, but if you include the same util changes across multiple PRs, you open the possibility to first 1) get the util changes merged with the least effort from the reviewer (i.e. with the lowest-reviewer-effort feature update) and then 2) update all the PRs so that the util changes aren't included in feature updates, which further reduces reviewer effort.

Oh, interesting—I've of course never been on the reviewer side in a meaningful capacity, but I had guessed the preference would be for the opposite! I'll keep that in mind for the future.

@marcustyphoon marcustyphoon merged commit e869607 into AprilSylph:master Apr 22, 2025
2 checks passed
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