Add reportPost GraphQL mutation for flag system#236
Conversation
Implements the database layer for issue hackers-pub#192 (flag/report system). Schema supports both post and actor reporting with duplicate prevention via unique constraints.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughA new reporting feature: database table Changes
Sequence DiagramsequenceDiagram
participant User as "User"
participant GraphQL as "GraphQL Resolver"
participant Models as "Flag Model"
participant DB as "Database"
User->>GraphQL: reportPost(postId, reason)
GraphQL->>GraphQL: require ctx.account
alt Not authenticated
GraphQL-->>User: NotAuthenticatedError
else
GraphQL->>DB: SELECT post (by id, actorId)
alt Post missing
GraphQL-->>User: InvalidInputError("postId")
else
alt reporter is post author
GraphQL-->>User: InvalidInputError("postId")
else
GraphQL->>Models: createFlag(iri, reporterId, postId, reason)
Models->>DB: INSERT INTO flag ON CONFLICT DO NOTHING
alt Insert succeeded
Models-->>GraphQL: { created: true, flagId }
GraphQL->>DB: SELECT post (for payload)
GraphQL-->>User: ReportPostPayload { post }
else
Models->>DB: SELECT id FROM flag WHERE reporter_id=... AND post_id=...
Models-->>GraphQL: { created: false, flagId }
GraphQL-->>User: AlreadyReportedError
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a post reporting feature, adding a flag table to the database and a reportPost mutation to the GraphQL API. The implementation includes logic to prevent duplicate reports and handle authentication. Feedback highlights a critical ID mismatch between the generated IRI and the database record, suggesting that the UUID be passed explicitly to the creation function. There are also recommendations to use explicit aliases in the table relations to resolve ambiguity between the reporter and the flagged actor.
| export async function createFlag( | ||
| db: Database, | ||
| iri: string, | ||
| reporterId: Uuid, | ||
| postId: Uuid, | ||
| reason: string, | ||
| ): Promise<CreateFlagResult> { | ||
| const id = generateUuidV7(); | ||
| const rows = await db.insert(flagTable) | ||
| .values({ id, iri, reporterId, postId, reason }) | ||
| .onConflictDoNothing() | ||
| .returning({ id: flagTable.id }); |
There was a problem hiding this comment.
The createFlag function generates a new UUID for the id field, but the iri passed by the caller already contains a UUID (generated in reportPost). This results in a mismatch between the flag's primary key and the ID referenced in its IRI. The function should accept the id as an argument to ensure consistency.
| export async function createFlag( | |
| db: Database, | |
| iri: string, | |
| reporterId: Uuid, | |
| postId: Uuid, | |
| reason: string, | |
| ): Promise<CreateFlagResult> { | |
| const id = generateUuidV7(); | |
| const rows = await db.insert(flagTable) | |
| .values({ id, iri, reporterId, postId, reason }) | |
| .onConflictDoNothing() | |
| .returning({ id: flagTable.id }); | |
| export async function createFlag( | |
| db: Database, | |
| id: Uuid, | |
| iri: string, | |
| reporterId: Uuid, | |
| postId: Uuid, | |
| reason: string, | |
| ): Promise<CreateFlagResult> { | |
| const rows = await db.insert(flagTable) | |
| .values({ id, iri, reporterId, postId, reason }) | |
| .onConflictDoNothing() | |
| .returning({ id: flagTable.id }); |
There was a problem hiding this comment.
Fixed — createFlag now accepts id as a parameter so it matches the IRI.
| const result = await createFlag( | ||
| ctx.db, | ||
| iri, | ||
| ctx.account.actor.id, | ||
| post.id, | ||
| args.input.reason, | ||
| ); |
There was a problem hiding this comment.
Pass the generated flagId to createFlag to ensure the database record's ID matches the one used in the IRI.
| const result = await createFlag( | |
| ctx.db, | |
| iri, | |
| ctx.account.actor.id, | |
| post.id, | |
| args.input.reason, | |
| ); | |
| const result = await createFlag( | |
| ctx.db, | |
| flagId, | |
| iri, | |
| ctx.account.actor.id, | |
| post.id, | |
| args.input.reason, | |
| ); |
There was a problem hiding this comment.
Fixed — flagId is now passed through to createFlag.
| posts: r.many.postTable(), | ||
| pins: r.many.pinTable(), | ||
| votedPolls: r.many.pollTable(), | ||
| flags: r.many.flagTable(), |
There was a problem hiding this comment.
The flags relation in actorTable is ambiguous because flagTable has two relations to actorTable (reporter and flaggedActor). You should disambiguate them using aliases. For example, use reportedFlags with the reporter alias.
| flags: r.many.flagTable(), | |
| reportedFlags: r.many.flagTable({ alias: "reporter" }), |
There was a problem hiding this comment.
Fixed — split into reportedFlags (alias reporter) and receivedFlags (alias flaggedActor) on actorTable, matching the blockingTable pattern.
| reporter: r.one.actorTable({ | ||
| from: r.flagTable.reporterId, | ||
| to: r.actorTable.id, | ||
| optional: false, | ||
| }), |
There was a problem hiding this comment.
Add an explicit alias to the reporter relation to allow disambiguation in the actorTable relations.
reporter: r.one.actorTable({
alias: "reporter",
from: r.flagTable.reporterId,
to: r.actorTable.id,
optional: false,
}),There was a problem hiding this comment.
Fixed — added alias: "reporter" to the reporter relation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
graphql/report.ts (1)
77-89: Minor: Non-null assertion on refetched post.Line 85 uses
post!which could fail if the post is deleted between mutation resolution and output field resolution. This is a rare race condition but could cause an unhandled error.Consider returning
nulland making thepostfield nullable, or handling the edge case explicitly.Optional: Handle deleted post edge case
post: t.drizzleField({ - type: Post, + type: Post, + nullable: true, async resolve(query, result, _args, ctx) { const post = await ctx.db.query.postTable.findFirst( query({ where: { id: result.postId } }), ); - return post!; + return post ?? null; }, }),Note: This would require updating
ReportPostPayload.postto be nullable in the schema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@graphql/report.ts` around lines 77 - 89, The output field resolver for post in outputFields uses a non-null assertion (post!) which can throw if the record was deleted between mutation and resolution; update the resolver in outputFields -> post (the async resolve function) to return post || null instead of asserting, and update the GraphQL type for ReportPostPayload.post to be nullable (or else handle the missing post by throwing a controlled error), ensuring callers and schema reflect the nullable post.models/flag.ts (2)
10-21: Consider acceptingidas a parameter for IRI consistency.The function generates its own UUID for
id(line 17), but the caller ingraphql/report.tsgenerates a separateflagIdfor the IRI path. This means the flag's actualiddiffers from the UUID embedded in itsiri.While this works, it's unusual for the IRI to reference a different UUID than the record's primary key. Consider accepting
idas a parameter to ensure consistency:Optional: Accept id as parameter
export async function createFlag( db: Database, + id: Uuid, iri: string, reporterId: Uuid, postId: Uuid, reason: string, ): Promise<CreateFlagResult> { - const id = generateUuidV7(); const rows = await db.insert(flagTable)Then in
graphql/report.ts:- const result = await createFlag( - ctx.db, - iri, + const result = await createFlag( + ctx.db, + flagId, + iri,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/flag.ts` around lines 10 - 21, The createFlag function currently calls generateUuidV7() internally so the stored primary key can differ from the UUID embedded in the IRI; change createFlag signature to accept an id parameter (e.g., id: Uuid) instead of generating one, update all callers to pass the same flagId they use for building the IRI (the caller that currently creates flagId should pass it into createFlag), and remove generateUuidV7() usage inside createFlag so flagTable.insert uses the provided id to ensure the DB id matches the IRI UUID.
27-32: Non-null assertion may cause runtime error on edge cases.If
onConflictDoNothing()is triggered by a conflict on theiriunique constraint (rather than the(reporterId, postId)constraint), the subsequentfindFirstquery by(reporterId, postId)will returnnull, causing a runtime crash atexisting!.id.While IRI collisions are extremely unlikely with UUIDv7, consider adding defensive handling:
Proposed defensive handling
const existing = await db.query.flagTable.findFirst({ columns: { id: true }, where: { reporterId, postId }, }); - return { created: false, flagId: existing!.id }; + if (existing == null) { + throw new Error("Flag conflict detected but no existing flag found"); + } + return { created: false, flagId: existing.id };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/flag.ts` around lines 27 - 32, The code assumes existing is non-null after onConflictDoNothing() and uses existing!.id, which can crash if the conflict was on the iri unique constraint; update the logic around db.query.flagTable.findFirst to defensively handle a null result: after the conflict, check if existing is null and if so re-query the flag table by the iri unique key (or run a broader lookup) to locate the record, and only then return { created: false, flagId: ... }; if no record is found return { created: false, flagId: null } instead of using a non-null assertion. Ensure you update the code paths that reference existing (the existing variable and the return) so they handle null safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@models/relations.ts`:
- Line 78: The flags relation is ambiguous because flagTable has two foreign
keys to actorTable (reporterId and actorId); update the relation definition for
flags (currently `flags: r.many.flagTable()`) to disambiguate by adding an alias
so Drizzle knows to use the reporterId FK — e.g., change the relation to use `{
alias: "reporter" }` on the flagTable relation to match the pattern used for
followingTable/blockingTable.
---
Nitpick comments:
In `@graphql/report.ts`:
- Around line 77-89: The output field resolver for post in outputFields uses a
non-null assertion (post!) which can throw if the record was deleted between
mutation and resolution; update the resolver in outputFields -> post (the async
resolve function) to return post || null instead of asserting, and update the
GraphQL type for ReportPostPayload.post to be nullable (or else handle the
missing post by throwing a controlled error), ensuring callers and schema
reflect the nullable post.
In `@models/flag.ts`:
- Around line 10-21: The createFlag function currently calls generateUuidV7()
internally so the stored primary key can differ from the UUID embedded in the
IRI; change createFlag signature to accept an id parameter (e.g., id: Uuid)
instead of generating one, update all callers to pass the same flagId they use
for building the IRI (the caller that currently creates flagId should pass it
into createFlag), and remove generateUuidV7() usage inside createFlag so
flagTable.insert uses the provided id to ensure the DB id matches the IRI UUID.
- Around line 27-32: The code assumes existing is non-null after
onConflictDoNothing() and uses existing!.id, which can crash if the conflict was
on the iri unique constraint; update the logic around
db.query.flagTable.findFirst to defensively handle a null result: after the
conflict, check if existing is null and if so re-query the flag table by the iri
unique key (or run a broader lookup) to locate the record, and only then return
{ created: false, flagId: ... }; if no record is found return { created: false,
flagId: null } instead of using a non-null assertion. Ensure you update the code
paths that reference existing (the existing variable and the return) so they
handle null safely.
🪄 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
Run ID: 750953b4-bd20-47c1-bbff-4d6166aeee7f
📒 Files selected for processing (8)
drizzle/0088_flag.sqlgraphql/mod.tsgraphql/report.tsgraphql/schema.graphqlmodels/deno.jsonmodels/flag.tsmodels/relations.tsmodels/schema.ts
Registers the reportPost relay mutation with duplicate detection (AlreadyReportedError) and self-report prevention.
malkoG
force-pushed
the
feature/flag-endpoint-with-minimum-requirement
branch
from
b59b8db to
3e30387
Compare
Add explicit aliases to flagTable actor relations to match the blockingTable pattern, avoiding ambiguity from two FKs to actorTable.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
models/relations.ts (1)
78-78:⚠️ Potential issue | 🟡 MinorAdd alias to disambiguate the foreign key relation.
The
flagTablehas two foreign keys toactorTable:reporterId(the reporter) andactorId(the flagged actor). Without an alias, Drizzle cannot determine which FK ther.many.flagTable()relation should use. Follow the pattern used forfollowingTableandblockingTablerelations.🔧 Proposed fix
- flags: r.many.flagTable(), + flags: r.many.flagTable({ alias: "reporter" }),And update the
flagTablereporter relation to include the matching alias:reporter: r.one.actorTable({ + alias: "reporter", from: r.flagTable.reporterId, to: r.actorTable.id, optional: false, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/relations.ts` at line 78, Add an explicit alias to the actor -> flags relation so Drizzle can pick the correct foreign key: change the relation call r.many.flagTable() to include an alias option (matching the pattern used by followingTable and blockingTable), and then update the corresponding reporter relation definition on flagTable to use the same alias so the reporterId FK is unambiguous.
🧹 Nitpick comments (4)
drizzle/0088_flag.sql (1)
1-17: Consider adding indexes for query performance.The table has unique constraints that will create indexes for
(reporter_id, post_id)and(reporter_id, actor_id), but if you need to query flags bypost_idalone (e.g., listing all reports for a post in a moderation dashboard), an additional index onpost_idwould improve performance.🔧 Optional index addition
ALTER TABLE "flag" ADD CONSTRAINT "flag_actor_id_actor_id_fk" FOREIGN KEY ("actor_id") REFERENCES "public"."actor"("id") ON DELETE cascade ON UPDATE no action; +--> statement-breakpoint +CREATE INDEX "idx_flag_post_id" ON "flag" ("post_id");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drizzle/0088_flag.sql` around lines 1 - 17, Add an index on the "flag" table for the post_id column to speed queries filtering by post (e.g., create an index on "flag"(post_id)); locate the table/constraints related to "flag" (CONSTRAINT "flag_post_id_post_id_fk", columns "post_id") and add a non-unique index for post_id; optionally also add a separate index on "actor_id" if you expect frequent lookups by actor.graphql/report.ts (2)
48-54: Consider distinguishing "post not found" from "self-report" errors.Both cases throw
InvalidInputError("postId"), which makes it impossible for clients to distinguish between a non-existent post and attempting to report one's own post. This could hinder user feedback in the UI.💡 Alternative approach
You could either:
- Use different
inputPathvalues:InvalidInputError("postId")vsInvalidInputError("postId.self")- Create a dedicated error type for self-reporting (e.g.,
CannotReportOwnPostError)Option 1 (minimal change):
if (post.actorId === ctx.account.actor.id) { - throw new InvalidInputError("postId"); + throw new InvalidInputError("postId.selfReport"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@graphql/report.ts` around lines 48 - 54, The two error cases in graphql/report.ts both throw InvalidInputError("postId"), which prevents clients from distinguishing a missing post from a self-report; update the self-report branch (the check post.actorId === ctx.account.actor.id) to return a distinct error—either throw InvalidInputError("postId.self") or a new error type (e.g., CannotReportOwnPostError) so callers can tell "post not found" (post == null -> InvalidInputError("postId")) from "cannot report your own post" (post.actorId === ctx.account.actor.id -> distinct error).
81-85: Non-null assertion on post lookup in output field.If the post is deleted between mutation resolution and output field resolution,
post!will throw an unexpected error. While this is a narrow race window, adding defensive handling would be more robust.🛡️ Defensive handling suggestion
async resolve(query, result, _args, ctx) { const post = await ctx.db.query.postTable.findFirst( query({ where: { id: result.postId } }), ); - return post!; + if (post == null) { + throw new Error("Post was deleted"); + } + return post; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@graphql/report.ts` around lines 81 - 85, The resolver in resolve (graphql/report.ts) currently uses a non-null assertion on the post lookup ("post!") which can throw if the post was deleted; change it to handle a null result from ctx.db.query.postTable.findFirst({ where: { id: result.postId } }) by either returning null (or an appropriate empty value) or throwing a controlled GraphQL error (e.g., new Error or ApolloError) so resolution is deterministic; update the resolve return behavior/type accordingly and ensure callers/typedefs accept the nullable case instead of using the non-null assertion.models/flag.ts (1)
27-32: Non-null assertion could mask unexpected errors.The
existing!.idassertion on line 32 assumes that if the insert failed due to conflict, the existing record must be found. While this is true for the(reporter_id, post_id)unique constraint, theonConflictDoNothing()also covers theiriunique constraint. Although IRI collision is practically impossible with UUIDv7, adding defensive handling would make the code more robust.🛡️ Defensive handling suggestion
const existing = await db.query.flagTable.findFirst({ columns: { id: true }, where: { reporterId, postId }, }); - return { created: false, flagId: existing!.id }; + if (existing == null) { + throw new Error("Flag conflict but no existing record found"); + } + return { created: false, flagId: existing.id };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/flag.ts` around lines 27 - 32, The code currently uses a non-null assertion existing!.id after calling db.query.flagTable.findFirst; replace this with a safe null-check: call db.query.flagTable.findFirst({ columns: { id: true }, where: { reporterId, postId } }) and if existing is null, handle it defensively (e.g., attempt a secondary lookup by iri or throw a descriptive Error/log and return created: false with flagId: null) instead of asserting; avoid existing! and ensure any onConflictDoNothing() path that left no matching row is detected and reported using identifiers reporterId, postId, and iri.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@models/relations.ts`:
- Line 78: Add an explicit alias to the actor -> flags relation so Drizzle can
pick the correct foreign key: change the relation call r.many.flagTable() to
include an alias option (matching the pattern used by followingTable and
blockingTable), and then update the corresponding reporter relation definition
on flagTable to use the same alias so the reporterId FK is unambiguous.
---
Nitpick comments:
In `@drizzle/0088_flag.sql`:
- Around line 1-17: Add an index on the "flag" table for the post_id column to
speed queries filtering by post (e.g., create an index on "flag"(post_id));
locate the table/constraints related to "flag" (CONSTRAINT
"flag_post_id_post_id_fk", columns "post_id") and add a non-unique index for
post_id; optionally also add a separate index on "actor_id" if you expect
frequent lookups by actor.
In `@graphql/report.ts`:
- Around line 48-54: The two error cases in graphql/report.ts both throw
InvalidInputError("postId"), which prevents clients from distinguishing a
missing post from a self-report; update the self-report branch (the check
post.actorId === ctx.account.actor.id) to return a distinct error—either throw
InvalidInputError("postId.self") or a new error type (e.g.,
CannotReportOwnPostError) so callers can tell "post not found" (post == null ->
InvalidInputError("postId")) from "cannot report your own post" (post.actorId
=== ctx.account.actor.id -> distinct error).
- Around line 81-85: The resolver in resolve (graphql/report.ts) currently uses
a non-null assertion on the post lookup ("post!") which can throw if the post
was deleted; change it to handle a null result from
ctx.db.query.postTable.findFirst({ where: { id: result.postId } }) by either
returning null (or an appropriate empty value) or throwing a controlled GraphQL
error (e.g., new Error or ApolloError) so resolution is deterministic; update
the resolve return behavior/type accordingly and ensure callers/typedefs accept
the nullable case instead of using the non-null assertion.
In `@models/flag.ts`:
- Around line 27-32: The code currently uses a non-null assertion existing!.id
after calling db.query.flagTable.findFirst; replace this with a safe null-check:
call db.query.flagTable.findFirst({ columns: { id: true }, where: { reporterId,
postId } }) and if existing is null, handle it defensively (e.g., attempt a
secondary lookup by iri or throw a descriptive Error/log and return created:
false with flagId: null) instead of asserting; avoid existing! and ensure any
onConflictDoNothing() path that left no matching row is detected and reported
using identifiers reporterId, postId, and iri.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60034097-018c-46de-a3a3-4446a83eb808
📒 Files selected for processing (8)
drizzle/0088_flag.sqlgraphql/mod.tsgraphql/report.tsgraphql/schema.graphqlmodels/deno.jsonmodels/flag.tsmodels/relations.tsmodels/schema.ts
dahlia
force-pushed
the
main
branch
2 times, most recently
from
ff82b29 to
38a6e99
Compare
1 participant
Summary
Implements phase 1 of the flag/report system (#192) — a
reportPostGraphQL mutation that allows authenticated users to report posts with duplicate prevention.This is a minimal implementation to satisfy app store verification requirements (reporting/flagging capability is required for user-generated content apps). Further phases will add actor reporting, federation via ActivityPub Flag activity, and moderation workflows.
flagdatabase table with support for both post and actor reporting (actor reporting reserved for future use), unique constraints to prevent duplicate reports, and a check constraint ensuring exactly one target typecreateFlagmodel function withonConflictDoNothingfor safe duplicate handlingreportPostrelay mutation withAlreadyReportedError,InvalidInputError(post not found or self-report), andNotAuthenticatedErrorTest plan
deno task migrateto apply the new migrationreportPostmutation works via GraphQL playgroundAlreadyReportedErrorInvalidInputErrorNotAuthenticatedError