Conversation
…mpty rendered lists
…lity mentions are linked
…bility mentions are linked
…'s last_activity_at
Codecov Report❌ Patch coverage is
Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note for reviewers: test coverage of sessions_controller is sufficient for the changes, it's just that it's mostly been uncovered before the change. |
cellio
left a comment
There was a problem hiding this comment.
LGTM. Nice reworking of expanded posts! Wow, we sure were loading a lot of extra stuff -- pulling that in only when actually needed is a big win, and also makes that code easier to work with.
For things like this, where future work is needed, are they mentioned in existing issues or would it be worth adding issues for them? I suspect they will probably be done by you, and you already have them in your head, but would we know what needed to be done if we lost you / your time for any reason? More optimistically, might it allow others to pick up some of it to share the work? |
Nope, no existing ones that I am aware of - 6 & 7 all come as a result of investigating root causes for our recent instability issues. That said, it won't hurt to have a tracking issue with detailed info on the findings for other contributors - I'll likely get to it soon-ish (feel free to take over that part too if you'd like - I don't mind having less stuff to do deal with, of course 😅).
For sure, no intention to become a one-man army - I am quite stretched as it is |
I'm generally very happy to add issues to avoid things being forgotten, but for 6 and 7 I'm out of my depth and wouldn't know what to raise. I'm guessing it would be quicker for you to raise them than to explain to me what needs raising. For now I've raised an empty shell of an issue that links back here #1828 |
This PR includes a bunch more fixes and improvements across the project:
remember_me_tokencookie not being generated for 2FA users because we have a custom override for TOTP;thread_contentaction now ensures Rails setsLast-Modifiedheader based on the thread's latest activity timestamp (needed to fix our overzealous caching);/users/merequests now include aCache-Control: no cachedirective (part of work on solving caching issues);I don't have time to spare to do 6 and 7 for every affected resource right now, but it should at least be a step forward.