[2.x] Fixed tags mutation shows [deleted] in some cases, together with improvements

[edgeinfinity1]

Fixes #0000

As title. If a tag was deleted from a discussion, it will no longer be included in its relationship, causing the DiscussionTaggedPost shows [deleted].
I modernized the component, added loading indicator, and now it will fetch tags list if any uncached tag is mentioned in mutation.

Changes proposed in this pull request:

Reviewers should focus on:

Maybe adding deleted tags in discussions' relationship is a better solution? Though I don't think it's necessary.

Screenshot

Necessity

• Has the problem that is being solved here been clearly explained?
• If applicable, have various options for solving this problem been considered?
• For core PRs, does this need to be in core, or could it be in an extension?
• Are we willing to maintain this for years / potentially forever?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).
• Core developer confirmed locally this works as intended.
• Tests have been added, or are not appropriate here.

Required changes:

• Related documentation PR: (Remove if irrelevant)

[imorland]

Thanks for the PR! The core approach is sound — detecting uncached tags and re-fetching before rendering is the right direction. A few issues need addressing before this can merge:

Truly deleted tags still produce undefined

The commented-out .filter(Boolean) reveals an unresolved decision:

.map((id) => app.store.getById('tags', id))
// .filter(Boolean);

After the fetch, a tag that has been genuinely deleted from the database will still return undefined from getById — it simply won't be in the store. So the [deleted] rendering can still occur, just less frequently (only for truly deleted tags rather than uncached ones).

Please either:

• Uncomment .filter(Boolean) to silently drop deleted tags from the display, or
• Explicitly handle undefined entries in tagsLabel / the description text

No error handling on the fetch

If app.store.find('tags') rejects (network error, server error), this.loading stays true forever and the component shows a spinner indefinitely. Please add a .catch():

app.store.find('tags').then(afterFetch).catch(() => {
  this.loading = false;
  m.redraw();
});

Minor: hardcoded height: 62px

.EventPost.DiscussionTaggedPost.Post.Post--loading {
    height: 62px;
}

Magic number — fragile if the post layout changes. Consider using min-height or deriving from a variable instead.

[imorland]

Thanks for the PR! The core approach is sound — detecting uncached tags and re-fetching before rendering is the right direction. A few issues need addressing before this can merge:

Truly deleted tags still produce undefined

The commented-out .filter(Boolean) reveals an unresolved decision:

.map((id) => app.store.getById('tags', id))
// .filter(Boolean);

After the fetch, a tag that has been genuinely deleted from the database will still return undefined from getById — it simply won't be in the store. So the [deleted] rendering can still occur, just less frequently (only for truly deleted tags rather than uncached ones).

Please either:

• Uncomment .filter(Boolean) to silently drop deleted tags from the display, or
• Explicitly handle undefined entries in tagsLabel / the description text

No error handling on the fetch

If app.store.find('tags') rejects (network error, server error), this.loading stays true forever and the component shows a spinner indefinitely. Please add a .catch():

app.store.find('tags').then(afterFetch).catch(() => {
  this.loading = false;
  m.redraw();
});

Minor: hardcoded height: 62px

.EventPost.DiscussionTaggedPost.Post.Post--loading {
    height: 62px;
}

Magic number — fragile if the post layout changes. Consider using min-height or deriving from a variable instead.

[edgeinfinity1]

I appreciate your advice a lot and added some patches, but I still have a few questions.

So the [deleted] rendering can still occur, just less frequently (only for truly deleted tags rather than uncached ones).

Returning undefined by default renders [deleted] on frontend, but this could only happen if the tag actually no longer exists. Isn't that the expected behavior already? I think it might be better to inform users that some tags are included in mutation history, even if they no longer exist.

No error handling on the fetch

I patched that according to your guidance.

Minor: hardcoded height: 62px

I added this style to fix loading indicator being too tall, since it's around 100px by default. When I was looking further into the size calculation according to your advice, I found that it was pretty hard-coded already:


These means that the height of this component could only be exactly 22 + 20 * 2 = 62px if window width >=768px, and exactly 21 + 20 * 2 = 61px if window width <=767.98px. Making it max-height or min-height neither works nor makes sense. Using calculations or variables are also seemingly unnecessary.
This height only affects the loading indicators' visual position, not any layout. Should I add the 62/61 media switch, or just leave it be? Or if there's a better approach, please tell me.

All the testing and observation were done on 1.8.x stable. Please tell me if anything has changed in 2.x.

I'm looking forward to your reply.