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

[Vanilla?] Grouped notifications of type 'status' for deleted statuses fail #2802

Open
kescherCode opened this issue Jul 31, 2024 · 8 comments
Labels

Comments

@kescherCode
Copy link

kescherCode commented Jul 31, 2024

Steps to reproduce the problem

  1. Have a notification for a status that is deleted
  2. Have "New posts" on in notification filters (vanilla or glitch, doesn't matter)
  3. JavaScript error will occur

Expected behaviour

Notifications load without any issue

Actual behaviour

Oops! An unexpected error occurred.

Detailed description

No response

Mastodon instance

catcatnya.com, cts.kescher.at, local dev container with artificial data

Mastodon version

commit 3271765, but probably occurs since initial implementation

Browser name and version

Recent versions of both Chromium and Firefox

Operating system

Linux, Android

Technical details

Error is:

TypeError: Cannot read properties of null (reading 'id')
    at e (index.js:64:95)
    at Array.forEach (<anonymous>)
    at index.js:84:14
    at redux-thunk.legacy-esm.js:5:14
    at dispatch (redux.legacy-esm.js:371:38)
    at f (notification_groups.ts:62:5)
    at notification_groups.ts:79:5
    at typed_functions.ts:197:28
    at async typed_functions.ts:66:16
    at async createAsyncThunk.ts:336:13

Leading me to this line:

pushUnique(normalStatuses, normalizeStatus(status, getState().getIn(['statuses', status.id]), getState().get('local_settings')));

Here is a redacted JSON object that's part of the response of /api/v2_alpha/notifications?exclude_types[]=follow_request&exclude_types[]=favourite&exclude_types[]=update&exclude_types[]=reaction&exclude_types[]=mention&exclude_types[]=follow&exclude_types[]=admin.report&exclude_types[]=reblog&exclude_types[]=admin.sign_up&exclude_types[]=poll:

{
  "group_key": "ungrouped-1177432",
  "notifications_count": 1,
  "type": "status",
  "most_recent_notification_id": 1177432,
  "page_min_id": "1177432",
  "page_max_id": "1177432",
  "latest_page_notification_at": "2024-07-31T10:51:42.901Z",
  "sample_accounts": [
    {
      "id": "1337",
      "username": "redacted",
      "acct": "redacted",
      "display_name": "redacted",
      "locked": true,
      "bot": false,
      "discoverable": false,
      "indexable": false,
      "group": false,
      "created_at": "1970-01-01T00:00:00.000Z",
      "note": "redacted",
      "url": "redacted",
      "uri": "redacted",
      "avatar": "redacted",
      "avatar_static": "redacted",
      "header": "redacted",
      "header_static": "redacted",
      "followers_count": -1,
      "following_count": -1,
      "statuses_count": -1,
      "last_status_at": "1970-01-01",
      "hide_collections": true,
      "noindex": true,
      "emojis": [],
      "roles": [],
      "fields": [
        {
          "name": "redacted",
          "value": "redacted",
          "verified_at": null
      ]
    }
  ],
  "status": null
}

Note that usually, status is populated.

@kescherCode
Copy link
Author

Now, the question is whether status being null is the bug here, or such null statuses not being filtered from notifications

@kescherCode kescherCode changed the title [Potentially also vanilla issue] Grouped notifications of type 'status' for deleted statuses fail [Vanilla?] Grouped notifications of type 'status' for deleted statuses fail Jul 31, 2024
@kescherCode
Copy link
Author

kescherCode commented Jul 31, 2024

Given null checks are present for other notification types, I'd say it's the latter.

@kescherCode
Copy link
Author

Manually deleting all "status" type notifications where no status exists is a working workaround, too. There were over 5000 such notifications in the database since catcatnya.com has started to exist.

@ClearlyClaire
Copy link

Can you retry with the last update? I suspect it behaves better. But it sounds like there's something weird going on. We technically expect some orphaned notifications for various reasons (basically polymorphism and race conditions), so it is good to guard against null statuses in the clients, but over 5k seems a bit much.

@kescherCode
Copy link
Author

but over 5k seems a bit much.

5k notifications since 2022, for the entire userbase of 150+ monthly active users. I think that's not that bad. There are worse issues in Mastodon.

Can you retry with the last update?

As production yields no new such notifications since my report (since no one rapid fire posted-and-deleted), I've gone ahead and injected a fake entry to bring back the issue (and then, just as I did that, someone managed to actually do it again, causing two entries for two users, one of those being me), then upgraded. Result: Notifications no longer result in an error, so it no longer breaks. However, there is an empty notification whenever the status can't be found:

An empty status in-between two "just posted" notifications

Btw, here's a quick query to run to get any such notifications:

select * from notifications n where n.type='status' and not exists (select s.id from statuses s where n.activity_id = s.id);

@kescherCode
Copy link
Author

If you think the empty notification is not a big deal, feel free to close the bug.

@ClearlyClaire
Copy link

It's not a very big deal but it's still a bug that needs to be investigated, at the very least on the display side.

@ClearlyClaire
Copy link

I think it has been addressed with recent changes, although the underlying cause still needs to be investigated

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

No branches or pull requests

2 participants