Repository navigation
feat(moderation): ✨ scoped moderator permissions, moderation registry and report API - #872
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
React Doctor found 9 new issues in 5 files · 9 warnings · score 88 / 100 (Great) · 0 fixed · vs 9 warnings
Reviewed by React Doctor for commit |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fb604d46b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .where(and(eq(table.id, report.id), eq(table.version, report.version))); | ||
| report.version += 1; |
There was a problem hiding this comment.
When a report is mutated after stale is read but before this optimistic update, the WHERE version = ... can update zero rows, yet the code increments the in-memory version and returns the newly resolved scope as though it succeeded. authorizeReport then reloads a report that can still contain its old scope and authorizes the current request against that stale scope, so a moderator limited to the old resource can briefly access or manage a report after its target moved. Check the affected row and retry the scope synchronization before authorizing.
Useful? React with 👍 / 👎.
| if ( | ||
| (await countRecentReports(c, user.id)) >= | ||
| MODERATION_LIMITS.reportsPerUserPerHour | ||
| ) { |
There was a problem hiding this comment.
The hourly limit is checked by counting existing rows before the later insert, so concurrent requests for different targets can all observe a count below 20 and all succeed. A client can therefore bypass the documented per-user limit simply by submitting reports in parallel; the duplicate key only serializes submissions for the same target. Reserve the quota atomically or perform the count and insert under suitable locking/serialization.
Useful? React with 👍 / 👎.
| for (const target of targets) { | ||
| if (moderator[target.permissionModule]) continue; | ||
| moderator[target.permissionModule] = moderationPermissionEntries(target); |
There was a problem hiding this comment.
If a plugin registers multiple custom report targets with the same permissionModule, processing the first target creates the module and this check silently skips every later target. Validation does not reject shared modules, so a later target with another scope family becomes inaccessible to restricted moderators, while a later hideable target may never expose can_view_hidden or can_hide in the catalog. Aggregate compatible entries from all targets or reject incompatible module reuse during registry validation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7b4683448
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * once it is done. Unique, so one reporter can hold one open report per | ||
| * target even when two submissions race. | ||
| */ | ||
| openKey: t.varchar({ length: 255 }).unique(), |
There was a problem hiding this comment.
Increase this column or replace the concatenated key with a collision-safe composite/hashed representation. The API accepts pluginId and targetType values up to 100 characters and targetId values up to 64, so with a 10-digit reporter ID and separators, reportOpenKey can reach 277 characters. Filing a report for such an otherwise valid registered target causes PostgreSQL to reject the insert with value too long for type character varying(255), returning a server error instead of creating the report.
Useful? React with 👍 / 👎.
| const [holder] = key | ||
| ? await tx | ||
| .select({ id: core_moderation_reports.id }) | ||
| .from(core_moderation_reports) | ||
| .where(and(eq(core_moderation_reports.openKey, key))) | ||
| .limit(1) |
There was a problem hiding this comment.
Reserve the key atomically or catch the unique violation and reopen without it. A reporter can legitimately have multiple resolved reports for one target, and if moderators reopen two of them concurrently, both transactions can observe no holder here and then attempt to write the same unique openKey; one request consequently fails with an uncaught PostgreSQL 23505 and a 500 response, even though the intended behavior is for the second report to reopen with a null key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afe4935b00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| dependsOn: [MODERATION_PERMISSIONS.viewHidden], | ||
| permission: MODERATION_PERMISSIONS.hide, | ||
| ...scope, |
There was a problem hiding this comment.
For a custom target whose hide or unhide action declares a permission other than can_hide, the generated catalog still contains only can_hide. The workflow checks definition.permission, so sanitizeStaffGrants drops the undeclared permission and every restricted moderator is permanently denied the action. Generate or validate the permissions declared by the action definitions instead of hard-coding this one.
Useful? React with 👍 / 👎.
| snapshot.fields = snapshot.fields.slice(0, -1); | ||
| } | ||
|
|
||
| return snapshot; |
There was a problem hiding this comment.
When a custom target returns more than 256 KiB in non-field data such as authors or url, this loop eventually removes every field but the final return still emits an oversized snapshot. That defeats the documented snapshotMaxBytes bound and permits unexpectedly large JSONB rows; after exhausting fields, truncate or reject the remaining oversized capture data before returning.
Useful? React with 👍 / 👎.
| params: { family }, | ||
| query: { | ||
| cursor, | ||
| ids: ids?.join(","), |
There was a problem hiding this comment.
A saved scope may contain up to 500 resource IDs, but this sends the entire selection as one comma-separated query. /scope-resources/{family} rejects query strings over 4,000 characters and silently slices valid requests to 200 IDs, so sufficiently large supported selections cannot have all of their labels or missing resources resolved in the editor. Split the IDs into bounded batches or align both endpoint limits with the 500-ID grant schema.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80ec99e8e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const where = and( | ||
| viewableReportsCondition(c, moderator.set), | ||
| ...queueFilters({ assigned, status, userId: moderator.user.id }), |
There was a problem hiding this comment.
When a scoped target is moved through the built-in Content Engine editor, no repository call synchronizes its reports, and this query continues filtering on the stored old scope; unlike authorizeReport, the queue never calls syncReportTargetScope. Independently of the optimistic-update race already reported, a moderator limited to the old resource can therefore see the report title, reason, target ID, and assignment indefinitely until somebody opens the report or explicitly invokes the optional sync helper. Integrate scope synchronization into Content Engine moves or otherwise refresh scopes before returning queue metadata.
Useful? React with 👍 / 👎.
| }) | ||
| .from(core_users) | ||
| .where(or(...conditions)) | ||
| .limit(LIMIT); |
There was a problem hiding this comment.
On installations with more than 100 moderator candidates, this unconditional limit silently excludes eligible users, while the assignee endpoint offers no search or pagination and the UI renders only the returned array. Moderators outside the arbitrary first 100 consequently cannot be selected as assignees, and the missing ORDER BY also makes membership unstable; paginate/search this query instead of truncating it.
Useful? React with 👍 / 👎.
| await appendReportEvent(c.get("db"), { | ||
| actorId: moderator.user.id, | ||
| data: { body: note }, | ||
| reportId: report.id, | ||
| type: "note", |
There was a problem hiding this comment.
This creates a moderation note and immediately returns without emitting through c.get("events"), even though the adjacent edit and delete routes emit moderation.report.note_updated and moderation.report.note_deleted. Event-driven integrations can observe every note mutation except creation, contrary to the repository requirement that important create/update/delete actions emit events; add and document a note-created event after the insert succeeds.
AGENTS.md reference: AGENTS.md:L61-L61
Useful? React with 👍 / 👎.
…erators - Scoped moderator permissions, moderation registry and report API - ModCP with reports board, report sidebar and snapshot peek - Report button with blog and example plugin reporting, plus migration - ModCP labels, snapshot rendering and docs screenshots - Moderation guides, reference pages and cross-links - Regression tests for scopes, reports and snapshots - Parallel database queries with Promise.all Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7cf8f13 to
8188327
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (moved.length === 0) return current; | ||
|
|
||
| for (const report of moved) { | ||
| await c |
There was a problem hiding this comment.
React Doctor · react-doctor/async-await-in-loop (warning)
This for…of loop waits before starting the next iteration. If iterations perform independent asynchronous work, consider bounded concurrency; await syntax alone does not establish a speedup.
Fix → Consider concurrent calls only for independent asynchronous work. Shared queues or synchronous work may not benefit. Preserve resource limits, transaction ordering, and failure/cancellation semantics; observe all callback promises.
| .where(inArray(core_users.id, assigneeIds)); | ||
|
|
||
| for (const assigneeId of assigneeIds) { | ||
| const user = users.find(row => row.id === assigneeId); |
There was a problem hiding this comment.
React Doctor · react-doctor/js-index-maps (warning)
This gets slow as your list grows because array.find() runs inside a loop, so build a Map once before the loop for instant lookups
Fix → Build a Map once before the loop instead of calling array.find(...) inside it
| const lost = reports.filter(report => report.assigneeId === assigneeId); | ||
| await c.get("db").transaction(async tx => { | ||
| for (const report of lost) { | ||
| const [updated] = await tx |
There was a problem hiding this comment.
React Doctor · react-doctor/async-await-in-loop (warning)
This for…of loop waits before starting the next iteration. If iterations perform independent asynchronous work, consider bounded concurrency; await syntax alone does not establish a speedup.
Fix → Consider concurrent calls only for independent asynchronous work. Shared queues or synchronous work may not benefit. Preserve resource limits, transaction ordering, and failure/cancellation semantics; observe all callback promises.
| const scope = findModerationScope(registry, family); | ||
| const found = new Set( | ||
| scope | ||
| ? (await scope.definition.find(c, [...ids])).map(item => item.id) |
There was a problem hiding this comment.
React Doctor · react-doctor/async-await-in-loop (warning)
This for…of loop waits before starting the next iteration. If iterations perform independent asynchronous work, consider bounded concurrency; await syntax alone does not establish a speedup.
Fix → Consider concurrent calls only for independent asynchronous work. Shared queues or synchronous work may not benefit. Preserve resource limits, transaction ordering, and failure/cancellation semantics; observe all callback promises.
| }; | ||
|
|
||
| const advanced = model.advanced.fields.filter((name: string) => | ||
| definition.reports.snapshotFields.includes(name), |
There was a problem hiding this comment.
React Doctor · react-doctor/js-set-map-lookups (warning)
This scales poorly because array.includes() inside a loop scans the whole list every time. Use a Set for constant-time lookups.
Fix → Use a Set or Map when you check for the same items over and over. Array.includes/find scans the whole list each time
| fields.push({ | ||
| label: labels.field(name), | ||
| name, | ||
| ...(await snapshotValue(tx, descriptor.kind, value)), |
There was a problem hiding this comment.
React Doctor · react-doctor/async-await-in-loop (warning)
This for…of loop waits before starting the next iteration. If iterations perform independent asynchronous work, consider bounded concurrency; await syntax alone does not establish a speedup.
Fix → Consider concurrent calls only for independent asynchronous work. Shared queues or synchronous work may not benefit. Preserve resource limits, transaction ordering, and failure/cancellation semantics; observe all callback promises.
| value: PermissionStaffScope | undefined; | ||
| } | ||
|
|
||
| export const StaffScopeDialogContent = ({ |
There was a problem hiding this comment.
React Doctor · react-doctor/no-high-complexity-react-function (warning)
StaffScopeDialogContent has cyclomatic complexity 17, cognitive complexity 22, and maximum nesting depth 3, so its React logic is hard to understand and change. Extract independent branches into components or hooks.
Fix → Extract independent render branches and state logic into focused components or hooks until the control flow is easy to follow.
| <li key={item.id}> | ||
| <label className="hover:bg-muted/40 flex items-center gap-3 px-3 py-2"> | ||
| <Checkbox | ||
| checked={selected.includes(item.id)} |
There was a problem hiding this comment.
React Doctor · react-doctor/js-set-map-lookups (warning)
This scales poorly because array.includes() inside a loop scans the whole list every time. Use a Set for constant-time lookups.
Fix → Use a Set or Map when you check for the same items over and over. Array.includes/find scans the whole list each time
| ); | ||
| }; | ||
|
|
||
| export const ModcpReportsBoard = ({ |
There was a problem hiding this comment.
React Doctor · react-doctor/no-giant-component (warning)
Component "ModcpReportsBoard" is over 300 lines long, which is hard to read & change. Split it into a few smaller components.
Fix → Pull each section into its own component so the parent is easier to read, test, and change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8188327fcf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const result = await c.get("db").transaction(async tx => { | ||
| const outcome = await definition.run(c, { |
There was a problem hiding this comment.
When definition.available(current) is false—such as a second hide request or a custom target whose state disallows the action—a client can bypass the disabled UI and call this endpoint directly, which invokes run without checking availability. The included test target's run always reports a change, so repeated hide requests increment the report version and create duplicate action events. Load the current target state and reject unavailable actions before starting the transaction.
Useful? React with 👍 / 👎.
| for (const report of lost) { | ||
| const [updated] = await tx | ||
| .update(core_moderation_reports) | ||
| .set({ assigneeId: null, status: "new", version: report.version + 1 }) |
There was a problem hiding this comment.
When a target moves outside its assignee's scope, this path clears the assignment and writes an unassigned timeline entry but never emits moderation.report.assigned. Integrations subscribing to assignment changes therefore retain the old assignee indefinitely, unlike changes made through the claim and assignment routes. Emit the event after each successful transaction, using the rows that were actually updated.
AGENTS.md reference: AGENTS.md:L61-L61
Useful? React with 👍 / 👎.
1 participant
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01U58HQh7q7zUuoKUnmgeKW7