Repository navigation
[COOP-1050] Stream recent decisions from server - #1419
calebmcquaid wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
4 issues found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/src/webpages/dashboard/mrt/activityExportUrl.ts">
<violation number="1" location="client/src/webpages/dashboard/mrt/activityExportUrl.ts:30">
P3: The 400-day export limit is defined separately here and in `exportActivityCsv.ts`, and the message is hardcoded in the test too. Export the limit from a shared module so the client's disabled-button check and the server's 400 check cannot drift apart.</violation>
</file>
<file name="server/routes/moderation_activity/ModerationActivityRoutes.test.ts">
<violation number="1" location="server/routes/moderation_activity/ModerationActivityRoutes.test.ts:182">
P3: This `rejects.toThrow()` passes on any failure, including a pre-stream 500 from `loadLookups`. Assert that the response started with status 200 and that the error is a socket abort rather than an HTTP status error, so the test really covers the mid-stream path.</violation>
</file>
<file name="client/src/webpages/dashboard/mrt/ManualReviewRecentDecisions.tsx">
<violation number="1" location="client/src/webpages/dashboard/mrt/ManualReviewRecentDecisions.tsx:1049">
P2: The Download button now navigates to the export URL and never looks at the response. If the server returns a 4xx or 5xx, the user gets no in-page error and no loading state, and the browser's download handling decides what happens. Consider fetching the URL, checking `response.ok`, and only then creating the Blob and anchor, with the loading flag restored.</violation>
</file>
<file name="server/routes/moderation_activity/streamActivityCsv.ts">
<violation number="1" location="server/routes/moderation_activity/streamActivityCsv.ts:42">
P3: After the response closes during a backpressure wait, the loop still pulls one more page from `pages`, which runs a DB query for 200 rows before `isClosed()` returns. Check `isClosed()` after `writeChunk` and before requesting the next page, which requires driving the iterator manually and calling `return()` on early exit.</violation>
</file>
Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack
View guided diff | Turn on auto-fix | Re-trigger cubic
| a.download = includeActions | ||
| ? 'moderation-activity.csv' | ||
| : 'decisions.csv'; | ||
| a.download = ''; |
There was a problem hiding this comment.
P2: The Download button now navigates to the export URL and never looks at the response. If the server returns a 4xx or 5xx, the user gets no in-page error and no loading state, and the browser's download handling decides what happens. Consider fetching the URL, checking response.ok, and only then creating the Blob and anchor, with the loading flag restored.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At client/src/webpages/dashboard/mrt/ManualReviewRecentDecisions.tsx, line 1049:
<comment>The Download button now navigates to the export URL and never looks at the response. If the server returns a 4xx or 5xx, the user gets no in-page error and no loading state, and the browser's download handling decides what happens. Consider fetching the URL, checking `response.ok`, and only then creating the Blob and anchor, with the loading flag restored.</comment>
<file context>
@@ -1111,65 +1038,31 @@ export default function ManualReviewRecentDecisions() {
- a.download = includeActions
- ? 'moderation-activity.csv'
- : 'decisions.csv';
+ a.download = '';
+ a.href = buildActivityExportUrl({
+ ...buildFilterInput(unsavedFilterValue ?? {}),
</file context>
| return `${ACTIVITY_EXPORT_PATH}?${params.toString()}`; | ||
| } | ||
|
|
||
| const MAX_EXPORT_SPAN_DAYS = 400; |
There was a problem hiding this comment.
P3: The 400-day export limit is defined separately here and in exportActivityCsv.ts, and the message is hardcoded in the test too. Export the limit from a shared module so the client's disabled-button check and the server's 400 check cannot drift apart.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At client/src/webpages/dashboard/mrt/activityExportUrl.ts, line 30:
<comment>The 400-day export limit is defined separately here and in `exportActivityCsv.ts`, and the message is hardcoded in the test too. Export the limit from a shared module so the client's disabled-button check and the server's 400 check cannot drift apart.</comment>
<file context>
@@ -0,0 +1,47 @@
+ return `${ACTIVITY_EXPORT_PATH}?${params.toString()}`;
+}
+
+const MAX_EXPORT_SPAN_DAYS = 400;
+
+export function exportRangeProblem(
</file context>
| const addActiveSpan = vi.spyOn(deps.Tracer, 'addActiveSpan'); | ||
|
|
||
| await expect( | ||
| request.get(exportUrl({ view: 'DECISIONS' })).buffer(true), |
There was a problem hiding this comment.
P3: This rejects.toThrow() passes on any failure, including a pre-stream 500 from loadLookups. Assert that the response started with status 200 and that the error is a socket abort rather than an HTTP status error, so the test really covers the mid-stream path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At server/routes/moderation_activity/ModerationActivityRoutes.test.ts, line 182:
<comment>This `rejects.toThrow()` passes on any failure, including a pre-stream 500 from `loadLookups`. Assert that the response started with status 200 and that the error is a socket abort rather than an HTTP status error, so the test really covers the mid-stream path.</comment>
<file context>
@@ -0,0 +1,192 @@
+ const addActiveSpan = vi.spyOn(deps.Tracer, 'addActiveSpan');
+
+ await expect(
+ request.get(exportUrl({ view: 'DECISIONS' })).buffer(true),
+ ).rejects.toThrow();
+ await vi.waitFor(() => {
</file context>
| return; | ||
| } | ||
| await writeChunk(out, activityCsvHeader(includeActions)); | ||
| for await (const page of pages) { |
There was a problem hiding this comment.
P3: After the response closes during a backpressure wait, the loop still pulls one more page from pages, which runs a DB query for 200 rows before isClosed() returns. Check isClosed() after writeChunk and before requesting the next page, which requires driving the iterator manually and calling return() on early exit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At server/routes/moderation_activity/streamActivityCsv.ts, line 42:
<comment>After the response closes during a backpressure wait, the loop still pulls one more page from `pages`, which runs a DB query for 200 rows before `isClosed()` returns. Check `isClosed()` after `writeChunk` and before requesting the next page, which requires driving the iterator manually and calling `return()` on early exit.</comment>
<file context>
@@ -0,0 +1,54 @@
+ return;
+ }
+ await writeChunk(out, activityCsvHeader(includeActions));
+ for await (const page of pages) {
+ if (isClosed()) {
+ return;
</file context>
taobojlen
left a comment
There was a problem hiding this comment.
cubic uncovered some real bugs worth fixing; primarily the lack of permissioning is a blocker!
1ea9f7e to
e2ebfda
Compare
9350ecd to
4b2d8da
Compare
e2ebfda to
1606801
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
96aa1ce to
d90d10e
Compare
d90d10e to
e2a7c6e
Compare
Context & Requests for Reviewers
The Recent Decisions CSV used to be built in the browser, which capped it at 10,000 rows. This moves the export to the server. The new
GET /api/v1/moderation_activity/exportroute takes the same filters as the page, pages through the activity feed, and streams the CSV to the browser.Some notes:
Tests
iterateAll.Checklist
Only check items that apply to this PR; leave the rest unchecked.
If you changed anything user-facing (i.e. user interface or APIs):
Did you update related docs?
If the change is notable (refer to Keep a Changelog conventions):
Did you update CHANGELOG.md?
If you changed
db/src/scripts/**and usedCREATE TABLE,ADD COLUMN, orALTER COLUMN:Are as many columns marked
NOT NULLas possible? If some columns can sometimes be null depending on other columns, are thereCHECKconstraints capturing those relationships, and are these also reflected using unions in the associated Kysely types?If you added a new signal in
server/services/signalsService/signals/**:Did you classify every error case as a permanent error (
SignalPermanentError, no retry) or a normal error (retryable)? Any case where the signal can't determine a score should be aSignalPermanentError.Summary by cubic
Streams the Recent Decisions CSV export from a new
GET /api/v1/moderation_activity/exportroute instead of building it in the browser, so downloads are no longer capped at 10,000 rows.VIEW_MRT_DATApermission, rejects requests started from outside Coop (Sec-Fetch-Site), and returns 400 for unknown decision types. The CSV columns are unchanged, and the Download button is disabled until a valid range is set.Written for commit e2a7c6e. Summary will update on new commits.