Add additionalInfo to NCMEC reports; fix XML element ordering and Node 24 multipart submission#477
Merged
Merged
Conversation
juanmrad
requested review from
cassidyjames,
dom-notion,
julietshen and
vinaysrao1
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional report additionalInfo, retry orchestration and GraphQL surface, debug logging/dumps and payload builder, client keyboard-safety and dashboard retry UI, creator-synthesis for enqueue flows, ClickHouse identity query, FormData fixes, input validation, and many tests. ChangesNCMEC Additional Info, Escalation, and Debug Enhancements
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx (1)
720-730:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRender messages preview as JSX, not a template string.
This block uses a template literal containing JSX interpolation, which will render the text
[object Object]instead of the actual elements. When React elements are interpolated into template strings, they're coerced to their string representation rather than rendered as components.Suggested diff
- {selectedThreadsWithMessages.length > 0 - ? ` - <div className="!my-4 divider" /> - <div className="text-base font-bold">Messages</div> - ${selectedThreadsWithMessages.map((thread) => ( - <div key={thread.threadId}> - {thread.threadId}: {thread.reportedContent.length} reported - </div> - ))} - ` - : undefined} + {selectedThreadsWithMessages.length > 0 ? ( + <> + <div className="!my-4 divider" /> + <div className="text-base font-bold">Messages</div> + {selectedThreadsWithMessages.map((thread) => ( + <div key={thread.threadId}> + {thread.threadId}: {thread.reportedContent.length} reported + </div> + ))} + </> + ) : undefined}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx` around lines 720 - 730, The current conditional uses a template string which coerces React elements to text; change it to render JSX directly by removing the backticks and string interpolation and returning a React fragment or element when selectedThreadsWithMessages.length > 0; e.g., replace the entire ternary expression with: selectedThreadsWithMessages.length > 0 ? (<><div className="!my-4 divider" /><div className="text-base font-bold">Messages</div>{selectedThreadsWithMessages.map(thread => (<div key={thread.threadId}>{thread.threadId}: {thread.reportedContent.length} reported</div>))}</>) : undefined, ensuring you keep the key (thread.threadId) and use thread.reportedContent.length.
🧹 Nitpick comments (3)
client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECInspectedMedia.tsx (1)
24-24: ⚡ Quick winUse
@/utils/miscinstead of deep relative import.This import should use the project alias to match path conventions.
Suggested diff
-import { isTypingInEditableElement } from '../../../../../../utils/misc'; +import { isTypingInEditableElement } from '@/utils/misc';As per coding guidelines,
**/*.{ts,tsx}: Use absolute imports with@/prefix over relative paths where configured intsconfig.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECInspectedMedia.tsx` at line 24, Replace the deep relative import of isTypingInEditableElement with the project alias import to follow tsconfig absolute paths: update the import in NCMECInspectedMedia.tsx so it imports isTypingInEditableElement from '@/utils/misc' instead of '../../../../../../utils/misc' (keep the imported symbol name isTypingInEditableElement unchanged).client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx (1)
33-33: ⚡ Quick winUse alias import for shared utility.
Replace the new deep relative import with the configured
@/alias.Suggested diff
-import { isTypingInEditableElement } from '../../../../../../utils/misc'; +import { isTypingInEditableElement } from '@/utils/misc';As per coding guidelines,
**/*.{ts,tsx}: Use absolute imports with@/prefix over relative paths where configured intsconfig.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx` at line 33, The import in NCMECReviewUser.tsx uses a deep relative path for the shared utility isTypingInEditableElement; replace that relative import with the configured alias import (use '@/utils/misc') so the line importing isTypingInEditableElement uses the `@/` prefix instead of '../../../../../../utils/misc' to follow the project absolute-import convention.client/src/components/common/CoopSelect.tsx (1)
5-5: ⚡ Quick winUse alias import for
miscutility.The new import uses a relative path; switch it to the configured
@/alias.Suggested diff
-import { isTypingInEditableElement } from '../../utils/misc'; +import { isTypingInEditableElement } from '@/utils/misc';As per coding guidelines,
**/*.{ts,tsx}: Use absolute imports with@/prefix over relative paths where configured intsconfig.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/components/common/CoopSelect.tsx` at line 5, Replace the relative import for the misc utility with the project alias: update the import of isTypingInEditableElement to use the configured "@/utils/misc" alias instead of "../../utils/misc" so the component CoopSelect.tsx imports isTypingInEditableElement from "@/utils/misc".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx`:
- Around line 302-306: The current lookup in NCMECReviewUser.tsx uses
ncmecDecisions.find to match only media.contentItem.id === decision.id and
media.contentItem.type.id === decision.typeId, which can misapply labels for
items with multiple media URLs; update the predicate in the ncmecDecisions.find
call (the decision lookup) to also compare the specific media URL(s) — e.g.,
match media.url (or the specific entry in media.contentItem.urls) against
decision.url (or decision.mediaUrl) — so decisions are only applied when id,
typeId and the exact media URL match.
In `@client/src/webpages/settings/NCMECSettings.tsx`:
- Around line 161-168: The client currently trims and format-validates the NCMEC
contact email (settings.contactEmail -> trimmedContactEmail) but lacks a
max-length check, so overly long emails can be sent to the server; add a
254-character limit check before the regex test (use trimmedContactEmail.length
<= 254), and if it exceeds 254 call toast.error with a clear message and return
from the submit/validation flow (update the validation block that contains
trimmedContactEmail and the regex test inside NCMECSettings.tsx).
In `@server/services/ncmecService/ncmecReporting.ts`:
- Around line 1228-1231: The current lexicographic comparison between
nowMinusOneSec and maxCreatedAt can be wrong for non-canonical ISO strings;
instead parse timestamps to numeric epoch ms and clamp using numeric comparison.
Convert maxCreatedAt to a Date/epoch (handle invalid dates), compute nowMs =
Date.now(), compute clampedMs = Math.min(maxCreatedAtMs, nowMs - 1000) and then
set clampedIncidentDateTime = new Date(clampedMs).toISOString(); update the
logic around nowMinusOneSec, maxCreatedAt, and clampedIncidentDateTime (and any
code that sets incidentDateTime) to use this numeric/date comparison and fall
back safely if maxCreatedAt is invalid.
---
Outside diff comments:
In
`@client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx`:
- Around line 720-730: The current conditional uses a template string which
coerces React elements to text; change it to render JSX directly by removing the
backticks and string interpolation and returning a React fragment or element
when selectedThreadsWithMessages.length > 0; e.g., replace the entire ternary
expression with: selectedThreadsWithMessages.length > 0 ? (<><div
className="!my-4 divider" /><div className="text-base
font-bold">Messages</div>{selectedThreadsWithMessages.map(thread => (<div
key={thread.threadId}>{thread.threadId}: {thread.reportedContent.length}
reported</div>))}</>) : undefined, ensuring you keep the key (thread.threadId)
and use thread.reportedContent.length.
---
Nitpick comments:
In `@client/src/components/common/CoopSelect.tsx`:
- Line 5: Replace the relative import for the misc utility with the project
alias: update the import of isTypingInEditableElement to use the configured
"@/utils/misc" alias instead of "../../utils/misc" so the component
CoopSelect.tsx imports isTypingInEditableElement from "@/utils/misc".
In
`@client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECInspectedMedia.tsx`:
- Line 24: Replace the deep relative import of isTypingInEditableElement with
the project alias import to follow tsconfig absolute paths: update the import in
NCMECInspectedMedia.tsx so it imports isTypingInEditableElement from
'@/utils/misc' instead of '../../../../../../utils/misc' (keep the imported
symbol name isTypingInEditableElement unchanged).
In
`@client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx`:
- Line 33: The import in NCMECReviewUser.tsx uses a deep relative path for the
shared utility isTypingInEditableElement; replace that relative import with the
configured alias import (use '@/utils/misc') so the line importing
isTypingInEditableElement uses the `@/` prefix instead of
'../../../../../../utils/misc' to follow the project absolute-import convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 864bb958-2bdf-4295-ac48-452398721ca6
📒 Files selected for processing (17)
client/src/components/common/CoopSelect.tsxclient/src/graphql/generated.tsclient/src/utils/misc.tsclient/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECInspectedMedia.tsxclient/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsxclient/src/webpages/settings/NCMECSettings.tsxserver/.env.exampleserver/graphql/generated.tsserver/graphql/modules/manualReviewTool.tsserver/graphql/modules/ncmec.tsserver/iocContainer/index.tsserver/services/hmaService/index.tsserver/services/manualReviewToolService/modules/JobDecisioning.tsserver/services/ncmecService/ncmecDebug.tsserver/services/ncmecService/ncmecEnqueueToMrt.tsserver/services/ncmecService/ncmecReporting.test.tsserver/services/ncmecService/ncmecReporting.ts
💤 Files with no reviewable changes (1)
- server/services/ncmecService/ncmecEnqueueToMrt.ts
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the NCMEC reporting flow end-to-end (UI → GraphQL → service → XML submission) to support the new additionalInfo field, fix NCMEC XML/XSD ordering and validation issues found in E2E testing, and adjust multipart submissions for Node 24/undici.
Changes:
- Add optional
additionalInfoto NCMEC reports and update MRT UI to separate “Escalate as High Priority” (checkbox + required reason) from additional notes. - Fix NCMEC submission correctness issues (XSD element ordering, optional/clean URL handling, incidentDateTime clamping, required reporter contact email) and improve error/debug observability.
- Update multipart upload handling to use undici
FormDatawithout manually setting multipart Content-Type.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/services/ncmecService/ncmecReporting.ts | Adds additionalInfo, enforces XSD ordering/validation constraints, improves submission logging/debug dumps, and switches to undici FormData types. |
| server/services/ncmecService/ncmecReporting.test.ts | Updates/extends unit tests for buildInternetDetailsFromOrgSetting to reflect URL omission behavior. |
| server/services/ncmecService/ncmecEnqueueToMrt.ts | Removes console logging on MRT enqueue success. |
| server/services/ncmecService/ncmecDebug.ts | Adds opt-in, non-production NCMEC debug logging + local payload dumps. |
| server/services/manualReviewToolService/modules/JobDecisioning.ts | Extends decision payload typing to include additionalInfo. |
| server/services/hmaService/index.ts | Uses undici FormData and avoids manually setting multipart Content-Type for uploads; general formatting cleanup. |
| server/iocContainer/index.ts | Plumbs additionalInfo from MRT decisions into NCMEC submission params. |
| server/graphql/modules/ncmec.ts | Adds server-side validation for required reporter contact email and trims credentials. |
| server/graphql/modules/manualReviewTool.ts | Extends MRT GraphQL input plumbing to include additionalInfo. |
| server/graphql/generated.ts | Regenerates types to include SubmitNcmecReportInput.additionalInfo. |
| server/.env.example | Documents NCMEC_DEBUG=1 opt-in debugging flag and its implications. |
| client/src/webpages/settings/NCMECSettings.tsx | Requires/validates contact email on save and marks the field required in the UI. |
| client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx | Adds additionalInfo textbox, changes escalation UX to checkbox + required reason, and prevents keyboard shortcuts while typing. |
| client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECInspectedMedia.tsx | Prevents keyboard shortcuts while typing in editable elements. |
| client/src/utils/misc.ts | Adds isTypingInEditableElement helper for keyboard shortcut guards. |
| client/src/graphql/generated.ts | Regenerates client types to include SubmitNcmecReportInput.additionalInfo. |
| client/src/components/common/CoopSelect.tsx | Prevents select keyboard shortcuts from triggering while typing in editable elements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/services/hmaService/index.ts (1)
835-841: 💤 Low valueRedundant defensive check can be simplified.
The guard at lines 809-813 already ensures that when
urlis absent, bothfileandcontentTypeare present. Since thiselsebranch is only reached whenurlis falsy, the condition!file || !contentTypeat line 837 should never be true.While defensive programming is reasonable, this check is unreachable. Consider either:
- Removing lines 837-841 entirely, or
- Keeping it but changing the error message to clarify it's an "impossible" state (e.g., "Internal error: ...")
♻️ Option 1: Remove the redundant check
} else { - // Guarded by the precondition above: when `url` is absent, both `file` - // and `contentType` are guaranteed to be set. - if (!file || !contentType) { - throw new Error( - 'addContentToBank reached file-upload branch without file/contentType', - ); - } + // Precondition: when `url` is absent, both `file` and `contentType` are guaranteed to be set. const formData = new FormData(); formData.append(contentType, file);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/hmaService/index.ts` around lines 835 - 841, The check inside addContentToBank that throws an error when `if (!file || !contentType) { throw new Error('addContentToBank reached file-upload branch without file/contentType') }` is redundant because earlier preconditions guarantee `file` and `contentType` when `url` is falsy; remove this unreachable conditional block entirely (or if you prefer to keep it as a sanity assertion, replace the message with an explicit impossible-state message like "Internal error: addContentToBank reached unreachable file-upload branch").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/services/hmaService/index.ts`:
- Around line 835-841: The check inside addContentToBank that throws an error
when `if (!file || !contentType) { throw new Error('addContentToBank reached
file-upload branch without file/contentType') }` is redundant because earlier
preconditions guarantee `file` and `contentType` when `url` is falsy; remove
this unreachable conditional block entirely (or if you prefer to keep it as a
sanity assertion, replace the message with an explicit impossible-state message
like "Internal error: addContentToBank reached unreachable file-upload branch").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ba83718-cf2a-43c4-bfa1-a860b6d47d5c
📒 Files selected for processing (7)
client/src/components/common/CoopSelect.tsxclient/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECInspectedMedia.tsxclient/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsxclient/src/webpages/settings/NCMECSettings.tsxserver/services/hmaService/index.tsserver/services/ncmecService/ncmecReporting.test.tsserver/services/ncmecService/ncmecReporting.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- client/src/components/common/CoopSelect.tsx
- client/src/webpages/settings/NCMECSettings.tsx
- client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx
- server/services/ncmecService/ncmecReporting.ts
…meralso
they don't byte-match across loads
julietshen
approved these changes
May 18, 2026
Member
julietshen
left a comment
julietshen
left a comment
There was a problem hiding this comment.
Excellent, thank you! Approving solely on the business logic and intended functionality - relying on copilot/coderabbit for code quality checks
…orts dashboard (#492) * [NCMEC] Persist failed submissions and surface retry in the NCMEC Reports dashboard * address code review comments by copilot and coderabbit * Make NCMEC and MRT enqueue work for users with no submission record (#494) * Make NCMEC and MRT enqueue work for users with no submission record * make it work with content type also * code review changes * code review feedback
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/services/ncmecService/ncmecReporting.ts (1)
1285-1302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact free-text report fields before logging
reportParams.
reportParamsnow carries reporter-enteredadditionalInfoand escalation text, but this method still serializes the whole object into span attributes and failure logs. That stores sensitive report content outside the debug-only path and creates an avoidable privacy/compliance risk.Suggested redaction
+ const logSafeReportParams = { + orgId: reportParams.orgId, + reviewerId: reportParams.reviewerId, + reportedUser: reportParams.reportedUser, + mediaCount: reportParams.media.length, + threadsCount: reportParams.threads.length, + incidentType: reportParams.incidentType, + hasEscalation: Boolean(reportParams.escalateToHighPriority?.trim()), + hasAdditionalInfo: Boolean(reportParams.additionalInfo?.trim()), + jobId: reportParams.jobId, + }; - span.setAttribute(`ncmecReportParams`, jsonStringify(reportParams)); + span.setAttribute( + `ncmecReportParams`, + jsonStringify(logSafeReportParams), + ); ... logErrorJson({ error: e, - message: jsonStringify({ reportParams, isTest }), + message: jsonStringify({ + reportParams: logSafeReportParams, + isTest, + }), });Also applies to: 1645-1666
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/ncmecService/ncmecReporting.ts` around lines 1285 - 1302, The span and debug logs in submitReport are serializing the entire reportParams (via jsonStringify and ncmecDebugLog) which can include reporter-entered free-text like additionalInfo and escalation text; redact or remove those sensitive fields before logging by cloning reportParams, deleting or replacing additionalInfo and any escalation/free-text fields with a constant like "[REDACTED]" and then use that sanitized object in span.setAttribute and ncmecDebugLog; update the same pattern wherever submitReport serializes/reportParams (including the later occurrence around the other span/log block) to ensure no free-text is stored in span attributes or non-debug logs.
♻️ Duplicate comments (2)
server/services/ncmecService/ncmecReporting.ts (1)
1323-1333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPick the latest
createdAtby parsed time, not by string order.
_.maxBy(reportParams.media, 'createdAt')still compares timestamp strings lexicographically. If anycreatedAtincludes offsets or non-canonical ISO formatting, you can still choose the wrong media timestamp even though the later clamp is numeric.Suggested fix
- const maxCreatedAt = _.maxBy( - reportParams.media, - 'createdAt', - )?.createdAt; + const maxCreatedAtMs = Math.max( + ...reportParams.media.map(({ createdAt }) => { + const ms = Date.parse(createdAt); + if (Number.isNaN(ms)) { + throw new Error( + `Invalid media createdAt timestamp for incidentDateTime: ${createdAt}`, + ); + } + return ms; + }), + ); + const maxCreatedAt = new Date(maxCreatedAtMs).toISOString();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/ncmecService/ncmecReporting.ts` around lines 1323 - 1333, The current max selection uses string comparison on createdAt; change the selection to pick the latest media by parsed time (numeric timestamp) instead: use _.maxBy(reportParams.media, m => Date.parse(m.createdAt) or new Date(m.createdAt).getTime()) and handle invalid/NaN parse results (throw same 'No media in report' or a clearer error). Replace the existing _.maxBy(...) usage that produces maxCreatedAt with this parsed-time comparator and keep the subsequent clampIncidentDateTimeToPast(maxCreatedAt) call using the original createdAt string from the chosen media (or convert consistently) so clampIncidentDateTimeToPast receives a valid datetime.client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx (1)
300-328:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid replaying one saved decision onto every URL for the same item.
Matching prior decisions on
(id, typeId)alone means a multi-URL content item will copy the first saved category/labels onto unrelated media. That makes the read-only NCMEC view show classifications that were never actually submitted. If URL matching is unstable, only prefill when the match is unambiguous; otherwise leave that media unselected until we have a stable per-media key.Suggested safe fallback
- const decision = ncmecDecisions.find( - (decision) => - media.contentItem.id === decision.id && - media.contentItem.type.id === decision.typeId, - ); + const decisionsForItem = ncmecDecisions.filter( + (decision) => + media.contentItem.id === decision.id && + media.contentItem.type.id === decision.typeId, + ); + const decision = + decisionsForItem.find( + (decision) => decision.url === media.urlInfo.url, + ) ?? + (decisionsForItem.length === 1 && + allMediaItemsWithUrls.filter( + (it) => + it.contentItem.id === media.contentItem.id && + it.contentItem.type.id === media.contentItem.type.id, + ).length === 1 + ? decisionsForItem[0] + : undefined);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx` around lines 300 - 328, The current mapping in NCMECReviewUser.tsx that builds items from allMediaItemsWithUrls applies a decision found by matching (decision.id, decision.typeId) which can incorrectly replay one saved decision across multiple URLs for the same content item; change the logic so you only prefill a decision when that decision is unambiguous: compute how many media entries share the same key (id + type) and only apply decision for a media if the matching decision exists AND the count for that (decision.id, decision.typeId) key is exactly 1; otherwise treat the media as no prior decision (category 'None', labels []). Update the mapping that references allMediaItemsWithUrls, ncmecDecisions, media.contentItem.id, media.contentItem.type.id, and decision.id/decision.typeId to use this uniqueness check before returning a prefilled object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/webpages/dashboard/ncmec/NcmecReportsDashboard.tsx`:
- Around line 427-433: The tableData useMemo currently swaps to
ncmecReportData.ncmecReportById whenever ncmecReportData exists and searchId is
truthy, which can show a stale previously-fetched report for a new search;
change the condition in the useMemo so you only substitute the lazy-fetched
single report when it actually matches the current searchId (e.g. verify
ncmecReportData.ncmecReportById.id or reportId equals searchId) and otherwise
fall back to ncmecReports; keep the existing fallback (ncmecReports ?? []) and
ensure the check uses the same identifier field present on ncmecReportById.
- Around line 7-8: The file uses React types such as React.ReactNode and
React.MouseEvent but only imports hooks; add an import of the React namespace to
satisfy those type references (e.g., import React from 'react') at the top of
NcmecReportsDashboard.tsx so that uses of React.ReactNode, React.MouseEvent and
any other React.* types resolve; update the same import in other affected
sections (lines ~15-24 and ~550-551) where those types are referenced.
In `@server/services/ncmecService/buildSubmitReportParamsFromDecision.ts`:
- Around line 108-122: decisionComponent.reportedMedia is currently matched
against allMediaItems using only contentItem.itemId which can pick the wrong row
when different content types share an id; update the lookup for reportedItem to
match both id and type (e.g., compare it.id to m.contentItem.itemId AND it.type
to m.contentItem.itemTypeIdentifier or equivalent), then proceed to call
getItemTypeEventuallyConsistent with the matched
reportedItem.contentItem.itemTypeIdentifier (ensure you update any error
messages referencing the lookup to reflect the combined id+type match); locate
the code around the reportedItem lookup and getItemTypeEventuallyConsistent to
implement this dual-key match.
---
Outside diff comments:
In `@server/services/ncmecService/ncmecReporting.ts`:
- Around line 1285-1302: The span and debug logs in submitReport are serializing
the entire reportParams (via jsonStringify and ncmecDebugLog) which can include
reporter-entered free-text like additionalInfo and escalation text; redact or
remove those sensitive fields before logging by cloning reportParams, deleting
or replacing additionalInfo and any escalation/free-text fields with a constant
like "[REDACTED]" and then use that sanitized object in span.setAttribute and
ncmecDebugLog; update the same pattern wherever submitReport
serializes/reportParams (including the later occurrence around the other
span/log block) to ensure no free-text is stored in span attributes or non-debug
logs.
---
Duplicate comments:
In
`@client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsx`:
- Around line 300-328: The current mapping in NCMECReviewUser.tsx that builds
items from allMediaItemsWithUrls applies a decision found by matching
(decision.id, decision.typeId) which can incorrectly replay one saved decision
across multiple URLs for the same content item; change the logic so you only
prefill a decision when that decision is unambiguous: compute how many media
entries share the same key (id + type) and only apply decision for a media if
the matching decision exists AND the count for that (decision.id,
decision.typeId) key is exactly 1; otherwise treat the media as no prior
decision (category 'None', labels []). Update the mapping that references
allMediaItemsWithUrls, ncmecDecisions, media.contentItem.id,
media.contentItem.type.id, and decision.id/decision.typeId to use this
uniqueness check before returning a prefilled object.
In `@server/services/ncmecService/ncmecReporting.ts`:
- Around line 1323-1333: The current max selection uses string comparison on
createdAt; change the selection to pick the latest media by parsed time (numeric
timestamp) instead: use _.maxBy(reportParams.media, m => Date.parse(m.createdAt)
or new Date(m.createdAt).getTime()) and handle invalid/NaN parse results (throw
same 'No media in report' or a clearer error). Replace the existing _.maxBy(...)
usage that produces maxCreatedAt with this parsed-time comparator and keep the
subsequent clampIncidentDateTimeToPast(maxCreatedAt) call using the original
createdAt string from the chosen media (or convert consistently) so
clampIncidentDateTimeToPast receives a valid datetime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e45025ae-3341-4327-863a-8e9e9bc7617d
📒 Files selected for processing (32)
client/src/components/ItemAction.tsxclient/src/graphql/generated.tsclient/src/webpages/dashboard/Dashboard.cssclient/src/webpages/dashboard/Dashboard.tsxclient/src/webpages/dashboard/components/table/Table.tsxclient/src/webpages/dashboard/mrt/manual_review_job/v2/ManualReviewJobMagnifyImageComponent.tsxclient/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECInspectedMedia.tsxclient/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECReviewUser.tsxclient/src/webpages/dashboard/ncmec/NcmecReportsDashboard.tsxserver/graphql/generated.tsserver/graphql/modules/ncmec.tsserver/graphql/modules/org.tsserver/iocContainer/index.tsserver/plugins/warehouse/queries/ClickhouseActionExecutionsAdapter.test.tsserver/plugins/warehouse/queries/ClickhouseActionExecutionsAdapter.tsserver/plugins/warehouse/queries/IActionExecutionsAdapter.tsserver/rule_engine/ActionPublisher.test.tsserver/rule_engine/ActionPublisher.tsserver/services/itemInvestigationService/index.tsserver/services/itemInvestigationService/itemInvestigationServiceAdapter.tsserver/services/itemInvestigationService/synthesizeUserItemFromContentTarget.tsserver/services/manualReviewToolService/manualReviewToolService.tsserver/services/manualReviewToolService/modules/JobDecisioning.tsserver/services/ncmecService/buildSubmitReportParamsFromDecision.tsserver/services/ncmecService/index.tsserver/services/ncmecService/ncmecReporting.tsserver/services/ncmecService/ncmecService.tsserver/services/ncmecService/ncmecSubmissionFilters.test.tsserver/services/ncmecService/ncmecSubmissionFilters.tsserver/services/ncmecService/retryNcmecSubmission.test.tsserver/services/ncmecService/retryNcmecSubmission.tsserver/workers_jobs/RetryFailedNcmecDecisionsJob.ts
✅ Files skipped from review due to trivial changes (4)
- server/services/ncmecService/ncmecSubmissionFilters.test.ts
- server/services/ncmecService/ncmecSubmissionFilters.ts
- client/src/graphql/generated.ts
- server/graphql/generated.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/webpages/dashboard/mrt/manual_review_job/v2/ncmec/NCMECInspectedMedia.tsx
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
server/workers_jobs/RetryFailedNcmecDecisionsJob.ts:130
submitReport()now recordsncmec_reports_errorsrows itself whenreportParams.jobIdis set (including forUNSUPPORTED_ORGandALL_MEDIA_MISSING). This job also callsinsertOrUpdateNcmecReportErrorfor those same outcomes, which will double-incrementretry_countand can cause retries to stop earlier than intended. Consider removing the extrainsertOrUpdateNcmecReportErrorwrites here (or only writing whenjobIdis not supplied tosubmitReport).
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Comment on lines
+2127
to
+2131
| try { | ||
| const status = opts.status ?? 'RETRYABLE_ERROR'; | ||
| // Atomic increment in `doUpdateSet` avoids the read-modify-write race | ||
| // when two retries land on the same job_id concurrently. | ||
| await this.pgQuery |
cassidyjames
changed the title
This was referenced May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #470
Context & Requests for Reviewers
Adds the
additionalInfofree-text field to NCMEC reports per the NCMEC report-details spec, and fixes a set of bugs that surfaced while end-to-end testing againstexttest.cybertip.org.Feature:
additionalInfoon the report (closes #470)SubmitNcmecReportInput.additionalInfo) → service → XML.<report>per the XSD.Fix: NCMEC XSD element ordering (responseCode=4100)
NCMEC rejects submissions whose elements aren't in XSD sequence order. js2xml emits children in object-literal key insertion order, so the
Reporttype and all builders now match the XSD sequence:added a new Opt-in, dev-only (
NODE_ENV !== 'production') structured logging plus a local XML dump underserver/ncmec-reports/to make future NCMEC validation debugging tractable. Documented inserver/.env.example. No-ops in production.Summary by CodeRabbit
New Features
Bug Fixes / Validation
Other