feat: 670 per reviewer skip - #893
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| ); | ||
| selectedRelatedActionsSetter(actions); | ||
| }; | ||
| const setSelectedRelatedActions = useCallback( |
There was a problem hiding this comment.
are you adding useCallback in response to an observed performance issue, or is it hypothetical?
| ); | ||
| selectedRelatedActionsSetter(actions); | ||
| }, | ||
| [actionStore], |
There was a problem hiding this comment.
should we not have selectedRelatedActionsSetter in the dependency array, too?
| setSelectedRelatedActions([]); | ||
| setDecisionReason(undefined); | ||
| }; | ||
| }, [setSelectedRelatedActions]); |
There was a problem hiding this comment.
why does this dependency array only have one of the functions?
|
|
||
| **Full Changelog**: https://github.com/roostorg/coop/compare/1.0.2...main | ||
|
|
||
| ## Review Console |
There was a problem hiding this comment.
(as in another PR) we should probably decide how we want to handle the changelog, i.e. writing to it per-PR or manually writing it all once in a while
| }); | ||
| // Release the reviewer's lock (the lock token is the reviewer's userId) | ||
| // so the job returns to the shared pool immediately for everyone else. | ||
| await this.releaseJobLock({ |
There was a problem hiding this comment.
are there any cases where we'd want to record a skip and not release the job lock? i can't think of any!
so if that's the case, can we call releaseJobLock from inside recordReviewerSkip? that way, callers won't have to remember to also release the lock manually each time they call it
| jobId: held.data.id, | ||
| lockToken, | ||
| jobId: convertedJob.data.id, | ||
| }).catch(() => {}); |
There was a problem hiding this comment.
the silent error catching is a little worrying! seems like we should at the very least log that, but i think it's fine to raise an exception here.
| return null; | ||
| } | ||
|
|
||
| static readonly REVIEWER_SKIP_TTL_MS = 30 * 60 * 1000; |
There was a problem hiding this comment.
hmm, how come we only skip for 30 minutes?
i guess in general -- what's the user story we imagine when someone skips a job? is it "i am not qualified to review this", "this is a hard job and i don't wanna deal with it / lower my average handling time by taking it", or something else? cc @julietshen
| queue, | ||
| user, | ||
| mrtService: container.ManualReviewToolService, | ||
| cleanup: async () => { |
There was a problem hiding this comment.
can you use the new transactional-backed test setup so we don't need to do the cleanup manually?
2 participants
Context & Requests for Reviewers
Part 3 of 3 for #670. Previously, skipping a job just released the lock. The job came right back to the same reviewer, and skip was two separate client calls (
logSkip+releaseJobLock) that could partially fail.logSkipatomically logs the skip, records the job in a per-reviewer Redis skip set (sorted set scored by expiry, with a TTL backstop), and releases the lock. The client's separateReleaseJobLockcall is removed.finally, so they return to the shared pool immediately for everyone else.getNextJobnow usesblock: false, so a drained queue returns null immediately instead of long-polling and hanging the reviewer's request.Review focus: the held-aside-then-release pattern in
dequeueNextJobWithLock, particularly lock-token semantics (the token is the reviewer's userId) and what happens if the release infinallypartially fails.Tests
QueueOperations.reviewerSkips.test.ts— skip set recording/expiry, dequeue filtering per reviewer, held-aside jobs released for other reviewers.Checklist
Did you update the CHANGELOG.md and related docs?