17

It is very annoying that you might be finished with reviewing a post (posted some comments) but you cannot submit feedback because the item is locked for review. I personally do not have the time or patience to keep refreshing the post until the lock is gone. I just did that for more than 10 minutes.

Instead allow me to submit the feedback but queue it. If the lock expires with no action - apply the feedback. If there is a change after the lock is released, then simply notify me, so I can have a look and know I can now submit feedback - be it the one I initially intended or a different feedback.

4
  • 6
    The experience, honestly, reminds of the days of when businesses put everything in a spreadsheet (we're pretending for the sake of the story that businesses have moved on), and someone had opened the file and then gone for lunch, leaving it open and locking everyone in the office out of file until they get home.
    – Thom A
    Commented Oct 17 at 8:48
  • What should happen if you queue an action, the review lock expires and the one holding the lock before submits an action afterwards? While that can already happen, it's currently pretty rare. I don't know whether it's possible to notify the reviewer previously holding the lock in that case.
    – dan1st
    Commented Oct 17 at 8:50
  • @dan1st then there was no change to the item from the lock that would make me reassess my feedback. The same situation happens without locks - if I am the first person to open an SG item, leave some feedback, then somebody else comes in and leaves feedback. I've evaluated the post based on what I currently see. I can't take into considerations things I've not seen.
    – VLAZ
    Commented Oct 17 at 8:57
  • @dan1st "the one holding the lock before submits an action afterwards" I think that is already handled. If there's an edit or some other review action since you've opened the page SG doesn't allow you to submit a review. Commented Oct 17 at 9:14

1 Answer 1

8

Let me say, broadly speaking, the goals of the "in review" system are to reduce frustration. While our review queue system essentially gets locks for free because we only hand a reviewer a single post at a time, it gets much more complicated when we allow reviewers in SG the flexibility to pick whatever post they want, but still simultaneously try to prevent multiple people from wasting their time by all reviewing the same exact post all at once.

It's not like we have a standup where all the SG reviewers can pick and choose who gets what ticket, and I know it can also be equally frustrating on the other end to spend a lot of time reviewing a ticket, making careful edits, only to have it scooped out from under you by someone who clicks "approve" right away. So there's some balance to maintain.

Let me at least detail the current behavior in this answer to at least inform more about how the system works and see if there are other opportunities on the UX side.

Current post locking logic

  1. When a reviewer opens a post, at that moment in time we try to get a lock on the post.
    1. If the post doesn't currently have a lock, the reviewer obtains one and can proceed with review
    2. If the post is already under review, we still allow it to be loaded, but add the đź”’ In Review label
  2. When the post is submitted, we again confirm if you have a lock.
    1. If you still have the one you claimed when loading the post, you're good to go.
    2. If you don't have a lock, but no-one else does either, you're good to go.
    3. If someone else has a lock, you'll get the error: A review for this post is currently being processed. Please try again
  3. Reviewers try to claim and need a lock to perform a review or an edit
  4. Authors will add a lock if they're editing their post, but are always allowed to submit edits, even if someone else is reviewing
  5. A lock is actively dismissed when
    1. The reviewer completes their review action (including skip)
    2. The reviewer clicks Return to Questions
    3. The author submits an edit or navigates off the edit page
    4. The reviewer or author closes the page or navigates away (relies on onbeforeunload)
  6. Locks are stored using redis and will automatically expire after 5 minutes of inactivity
    1. On the client we listens for keyup events in the title, editor, tag, or comments field and will submit refresh-or-try-claim-lock. This function is debounced every 30 seconds
  7. A list of all active locks is used to exclude posts under active review from
    1. the /staging-ground listing page
    2. the feeds on home & questions page
    3. loading the next reviewable post after a review
  8. If you have the page open and someone else makes an edit to the page, we should alert you to the change (via sockets), and ask you to refresh the page.
    1. Changes include edits to title, body, and tags and also changes to post state (i.e. New, Major Changes, etc)

I'll leave the rest of the question to our normal lifecycle, but wanted to at least document some extra details, in particular hopes that knowing the specifics might avoid some frustrating elements when you don't know what's going on or why.

12
  • "Locks are stored using redis" - FYI: nobody here has a need to know such implementation details. You may have overshared a little there.
    – Gimby
    Commented Oct 17 at 13:00
  • 1
    "When a reviewer opens a post, at that moment in time we try to get a lock on the post.", oh oops, good to know!, as I usually would open 2-3 Questions in their new Tab as I scroll down the main SG-Page before deciding (from the Title) which Question I will handle..., and only to switch to Tab2 or Tab3 if some other Reviewer was already handling the Thread on Tab1...
    – chivracq
    Commented Oct 17 at 13:10
  • 1
    But, hum the 5-min Lock doesn't seem to work/apply when a Post is in Re-evaluate State, see for example the following SG-Question and Comments + Actions within a few seconds between Reviewer 'Moe' and myself... (Not sure which one of us opened the Thread first, all I can say is that I didn't get any Lock myself...)
    – chivracq
    Commented Oct 17 at 13:55
  • @chivracq, It shouldn't have to do with re-evaluate shouldn't have anything to do with the locking. Reviewers are still allowed to comment while someone else has a lock. I don't see [multiple review actions] from you and Moe within a super tight timeframe.
    – KyleMit StaffMod
    Commented Oct 17 at 14:34
  • 1
    Maybe apply opacity (like you do for downvoted answers) to SG posts that also have the "In review" lock. It needs to be more prominent because I usually only notice a post is "locked' after I've spent a significant amount of time looking at it with the intent to provide a review/feedback.
    – TylerH
    Commented Oct 17 at 15:42
  • 5
    @Gimby On the contrary, we're all programmers here. So when we're requesting new features or changes to existing ones, it's often very useful to know some implementation details that could affect what kinds of changes are possible. If we know a change will only require updating a variable and an if condition, we're more likely to push for it. If we know a system that has a problem relies on complex integration with 6 other platforms, we're likely going to be way more understanding when it takes 6-8 months to fix. Would that all responses to bugs/feature reqs had this level of detail.
    – TylerH
    Commented Oct 17 at 15:45
  • @TylerH, ahh, yeah, good to know that the "In Review" label is not quite loud enough to let you know to move on or come back later. Even just as a user-scriptable style update, I wonder if something like this .mainbar-full:has(.iconLockSm) .s-prose:not(:hover), .mainbar-full:has(.iconLockSm) .js-review-actions-container:not(:hover) { opacity: 0.5; transition: 2s opacity ease-in; } is what you were thinking of (example)
    – KyleMit StaffMod
    Commented Oct 17 at 18:23
  • 1
    @KyleMit Yeah, something like that would be pretty noticeable and clear that "you shouldn't be reviewing this item right now", if not exactly noticeable on why (although the "skip" and "return to questions" buttons would need to be excluded from the opacity, if that kind of thing were implemented.
    – TylerH
    Commented Oct 17 at 18:45
  • 1
    "it gets much more complicated when we allow reviewers in SG the flexibility to pick whatever post they want" it's also complicated because the posts are forced onto users in the question lists. Yes, that's togglable but it's on by default. So the case of two users opening the same post is not that rare, nor is it just because two users chose this. The design makes them make this choice. At any rate, I understand the reason for a lock, however, I also know a lot of locks do not result in a review action.
    – VLAZ
    Commented Oct 18 at 9:05
  • @VLAZ, we do at least randomize the questions shown inline in questions and home feed and also exclude in review items from that as well. Totally agree that this will lead to the same post being show to multiple people, but possible much in the same way as if we had a lot of active reviewers in SG as well.
    – KyleMit StaffMod
    Commented Oct 18 at 13:46
  • Thinking out loud here - maybe we need to add eventing to when someone obtains a lock, so we can better understand the funnel of getting a lock, but not actually completing a review for a post and what the falloff rate is there.
    – KyleMit StaffMod
    Commented Oct 18 at 13:48
  • 1
    "we do at least randomize the questions shown inline in questions" OK but...the the randomness is not THAT random. We only get questions from the tags we're looking at. This is my custom filter. It currently shows five staging ground items in the list. Some days it's less, like 1-2. Two people looking at the same tags can easily land on the same SG item. If there were more candidates for the SG slots in the question lists, it'd sure be easier to avoid collisions. But that's not the case currently. Even then that still leaves SG itself.
    – VLAZ
    Commented Oct 18 at 13:52

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .