[386] feat: warehouse reads for manual moderator actions - #1027
[386] feat: warehouse reads for manual moderator actions#1027calebmcquaid 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 |
calebmcquaid
changed the title
|
The PR description here is not super helpful.
That's useful context, but:
This is kind of hard to grok without first reading the diff -- but that's the whole point of a PR description! It also doesn't really state what this PR is for, why it exists, etc. |
|
Also, the diff shown for this PR includes the diff for the downstack PR. Not sure why but that shouldn't happen -- each PR diff in a stack should be just its own changes. It's best to review your own PRs for stuff like that before handing them off! (Fortunately in this case the previous PR's changes were pretty self-contained so it doesn't interfere too much with reviewing this PR). |
| const MODERATOR_ACTION_SOURCES = ['manual-action-run']; | ||
|
|
||
| /** How far back a single page of the moderator action feed may scan. */ | ||
| const DEFAULT_ACTION_FEED_LOOKBACK_MS = 3 * MONTH_MS; |
There was a problem hiding this comment.
this seems like a pretty arbitrary limit to me. is it necessary? IMO we should allow viewing all actions regardless of how far back they were, that seems crucial?
| } | ||
|
|
||
| /** `YYYY-MM-DD`, for comparison against the `ds` partition column. */ | ||
| export function toDsString(date: Date): string { |
There was a problem hiding this comment.
we already have getUtcDateOnlyString in utils/time.ts that does this.
| * *local* time — which would shift every row on a non-UTC host and interleave | ||
| * this feed incorrectly against the Postgres decisions it merges with. | ||
| */ | ||
| export function parseWarehouseDateTime(value: string): Date { |
There was a problem hiding this comment.
we already have parseClickhouseTimestamp that does this, i think?
| * Writes go through `ClickhouseAnalyticsAdapter.formatDate`, which stores UTC | ||
| * wall-clock with no zone suffix (`YYYY-MM-DD HH:MM:SS.mmm`). | ||
| */ | ||
| export function formatWarehouseDateTime(date: Date): string { |
There was a problem hiding this comment.
this function isn't needed if you rewrite the clickhouse queries to use parseDateTime64BestEffort() like we do elsewhere!
| } | ||
|
|
||
| /** | ||
| * The `ds` partition range a moderator-action feed page must scan. |
There was a problem hiding this comment.
this comment is a bit confusing. it should state what ds is!
| * over the whole window while this would group over a narrower one, silently | ||
| * dropping items and their failures. Defaults to the feed's window. | ||
| */ | ||
| lookbackWindowMs?: number; |
There was a problem hiding this comment.
as above, is this strictly needed?
| * (e.g. `policies`, `rules`). Returns `null` for empty/absent/malformed input | ||
| * so callers can distinguish "nothing there" from "zero ids after filtering". | ||
| */ | ||
| export function parseJsonIdArray( |
There was a problem hiding this comment.
nit: i started reviewing this as new code and only later realized it was a refactoring that moved it here. it's best to separate feature work and refactoring as it can confuse reviewers (like me!)
no need to change anything here just a bit of feedback!
| // `ds` bounds the partition scan only. The cursor and `after` are applied | ||
| // in HAVING, against the complete group. | ||
| // | ||
| // Each bound carries a day of slack so it never truncates a group we |
There was a problem hiding this comment.
when you read this code comment, does it make complete sense to you?
i am asking because i am struggling to understand this and would appreciate a walkthrough!
| } | ||
|
|
||
| // Grouping by correlation_id collapses the one-row-per-(item, action) fan | ||
| // out into a single record per moderator operation. Without it, a bulk |
There was a problem hiding this comment.
what do you think of the UX here? you could argue that a bulk action on 500 ids really is 500 separate actions and thus should be shown separately... but i'm not sure.
| } = input; | ||
|
|
||
| // The `ds` bounds must span at least as much as the feed query's window, | ||
| // or a row and its own detail panel disagree. The feed groups a run over |
There was a problem hiding this comment.
a row and its own detail panel disagree
can you explain this?
2 participants
Context & Requests for Reviewers
Second of four for #386. Stacked on #A.
Bulk Actioning and Investigation write to
ACTION_EXECUTIONSbut produce no review decision. So those actions don't show up currently.getRecentModeratorActionsgroups bycorrelation_idso a 500-item run is one record, instead of 500 rows.getManualActionItemsreturns the items a run touched, with failure state. Both are scoped toaction_source = 'manual-action-run'.Tests
37 unit tests. Typechecks and passes standalone.