[386] feat: merge review decisions and manual actions into one - #1029
[386] feat: merge review decisions and manual actions into one#1029calebmcquaid wants to merge 2 commits into
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:
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 |
there's no such thing as #B -- it's very helpful if you write PR descriptions in your own words so we avoid things like this! when i see LLM-written stuff like this, especially stuff that has small mistakes, it has the effect that i don't trust any of the code or comments in the PR because i don't know if you've read over it yourself and can stand 100% behind it. i really don't mean to be rude! but i think it's important because it makes PR reviews much harder -- i don't know if i'm reviewing caleb's contribution or an LLM's contribution, you know? i trust the former a lot more :) |
| @@ -0,0 +1,35 @@ | |||
| import { typeDefs } from './moderationActivity.js'; | |||
|
|
|||
| describe('moderationActivity schema', () => { | |||
There was a problem hiding this comment.
these tests are kind of weird -- they're basically testing that some strings contain some substrings?
i don't think we need these!
| } from '../generated.js'; | ||
| import { forbiddenError, unauthenticatedError } from '../utils/errors.js'; | ||
|
|
||
| /** Matches the decisions feed's page size so the two merge evenly. */ |
There was a problem hiding this comment.
if these numbers need to match those from the decision feed, can we please use the same constants so there's no risk of drift?
| * panel keeps its truncation notice for that case rather than pretending the | ||
| * list is always complete. | ||
| */ | ||
| const MAX_ITEM_PAGE_SIZE = 1000; |
There was a problem hiding this comment.
i don't understand this. if there's a limit that's only enforced client-side, then what is this constant for? when the comment says "the panel", what is it referring to?
| * Maps the GraphQL oneof-style decision filter to the service's tagged-union | ||
| * shape. Mirrors the equivalent mapping in `manualReviewTool.ts`'s | ||
| * `getRecentDecisions` resolver — kept separate rather than shared, since | ||
| * touching that resolver is out of scope here. |
There was a problem hiding this comment.
please review your code and its comments before handing it off. this comment does not make sense to commit to the codebase, since it's clearly directed at the developer making changes, rather than future readers of the code.
There was a problem hiding this comment.
anyway, for this function, we also have getRecentDecisions and getSkipsForRecentDecisions in manualReviewTool.ts that do basically the same thing. i do think it's worth extracting this into a shared helper (can be done in a PR upstack).
| Math.max(1, input.limit ?? DEFAULT_PAGE_SIZE), | ||
| MAX_PAGE_SIZE, | ||
| ), | ||
| // Already decoded by the `Cursor` scalar; `parseActivityCursor` handles |
There was a problem hiding this comment.
as a reader of this comment my first question is "what is parseActivityCursor?".
IMO no need to reference it here.
| const UNUSED_PAGE = 0; | ||
|
|
||
| /** Merged-view lookback. See the spec's "Time window" section. */ | ||
| const MERGED_VIEW_WINDOW_MS = 30 * 24 * 60 * 60 * 1000; |
There was a problem hiding this comment.
this comment makes no sense as we do not commit specs to this repo. (anyway code comments should not require reading a separate spec to understand).
There was a problem hiding this comment.
but anyway, do we need this time window?
| * Manual actions are only ever taken from Investigation or Bulk Actioning, so | ||
| * seeing them requires the permission that gates Investigation itself. | ||
| * | ||
| * This is not a formality. `EXTERNAL_MODERATOR` — described in |
There was a problem hiding this comment.
again, let's stop referencing this role, as it may change.
| * The Recent Decisions feed, merged from two stores. | ||
| * | ||
| * Review-job decisions live in Postgres and manual moderator actions live in | ||
| * ClickHouse. Nothing outside this module knows that. |
There was a problem hiding this comment.
| * ClickHouse. Nothing outside this module knows that. | |
| * ClickHouse. |
unnecessary LLM fluff
| view: ActivityView; | ||
| limit: number; | ||
| /** Already decoded by the `Cursor` scalar; absent for the newest page. */ | ||
| cursor?: unknown; |
| /** | ||
| * Every item id one manual action run touched. | ||
| * | ||
| * Callers MUST check `VIEW_INVESTIGATION` first — see `canViewManualActions`. |
There was a problem hiding this comment.
do you find this comment easy to read?
2 participants
Context & Requests for Reviewers
Third of four for #386. Stacked on #B.
ModerationActivityFeedmerges PG review-job decisions with manual Clickhouse actions into one list.Two things to call out:
The cursor keeps a separate position per store. Decision ids are uuids, action ids are strings like
manual-action-run:abc. One shared cursor throws a PG type error on any page ending on an action, and casting to text doesn't help because JS, CH and PG each sort strings differently.Tests
24 unit tests, full server suite passes, typechecks standalone.