Conversation
📝 WalkthroughWalkthroughAdds three Express routers (claims, report-links, items), Zod validators, Prisma-backed handlers for claims and match-suggestions, atomic report-link consumption on submit, public item listing/stats endpoints, and mounts these routes in the main server; README updated to mark the endpoints Done. ChangesNew API Features
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
backend/README.md (1)
106-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the missing validator and route files in the project structure.
The project structure section adds
claims.tsbut omitsreportLinks.tsanditems.tsin both thevalidators/androutes/sections, even though these files exist (confirmed by the review stack context) and their endpoints are documented as "Done" in the API tables below.📝 Proposed additions to the project structure
Add the missing files to the validators section:
├── validators/ │ ├── shared.ts # validate() and validateQuery() middleware helpers │ ├── auth.ts # Zod schemas: loginSchema, registerSchema, refreshSchema, logoutSchema │ ├── claims.ts # Zod schemas for claim routes +│ ├── reportLinks.ts # Zod schemas for report-link routes +│ ├── items.ts # Zod schemas for item routes │ └── users.ts # Zod schemas: replaceProfileSchema, updateProfileSchema, createUserSchema, listUsersQuerySchemaAdd the missing files to the routes section:
├── routes/ │ ├── health.ts # GET /api/health │ ├── auth.ts # POST /api/auth/login|register (done) · refresh|logout (stub) │ ├── claims.ts # Claim lifecycle routes +│ ├── reportLinks.ts # Report link validation and found-item submission routes +│ ├── items.ts # Public item browsing and category statistics routes │ ├── users.ts # GET|PUT /api/users/me (done) · PATCH stubs │ └── admin/ │ └── users.ts # Admin user management stubs🤖 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 `@backend/README.md` around lines 106 - 127, Update the project structure in backend/README.md to include the omitted validator and route files: add validators/reportLinks.ts and validators/items.ts under the validators/ list and add routes/reportLinks.ts and routes/items.ts under the routes/ list so the README matches existing code and the API tables; ensure the short descriptions mirror the style used for other entries (e.g., mention which endpoints are done/stubbed) and keep formatting consistent with the surrounding tree.
🧹 Nitpick comments (10)
backend/src/validators/reportLinks.ts (1)
30-30: 💤 Low valueConsider moving
.trim()before.max(2000)for consistency.Currently,
.max(2000)validates the untrimmed input length. If the validation order matters for your use case (e.g., you want to validate the trimmed length), consider:z.string().trim().max(2000).optional().However, the current order prevents users from submitting strings that are >2000 chars even if they would trim down. This may be intentional to avoid processing unnecessarily long inputs.
🤖 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 `@backend/src/validators/reportLinks.ts` at line 30, The additionalNotes schema uses .max(2000) before .trim(), so length is checked on the untrimmed input; update the validator for additionalNotes (the z.string() chain) to call .trim() before .max(2000) — e.g., change the chain to z.string().trim().max(2000).optional() — so the trimmed length is what gets validated.backend/src/routes/claims.ts (2)
1017-1026: 💤 Low valueInconsistent error signaling pattern.
LINKED_ITEM_NOT_FOUNDsets only the message (line 1019), whileLINKED_ITEM_NOT_STOREDexplicitly sets both message and name (lines 1023-1025). The catch block then checks.namefor one and.messagefor the other (lines 1074, 1082). While functional, this inconsistency could lead to subtle bugs if refactored.Consider using a consistent pattern for both:
♻️ Suggested fix
if (!item) { - throw new Error('LINKED_ITEM_NOT_FOUND'); + const notFoundError = new Error('LINKED_ITEM_NOT_FOUND'); + notFoundError.name = 'LINKED_ITEM_NOT_FOUND'; + throw notFoundError; }Then update the catch block:
- if (err instanceof Error && err.message === 'LINKED_ITEM_NOT_FOUND') { + if (err instanceof Error && err.name === 'LINKED_ITEM_NOT_FOUND') {🤖 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 `@backend/src/routes/claims.ts` around lines 1017 - 1026, Make the error signaling consistent: when throwing LINKED_ITEM_NOT_FOUND and LINKED_ITEM_NOT_STORED set both error.message and error.name (e.g., new Error('LINKED_ITEM_NOT_FOUND') then error.name = 'LINKED_ITEM_NOT_FOUND' for the not-found case) so both throws use the same pattern; then update the catch block that currently mixes checks of .name and .message to check the same property (prefer checking .name for both 'LINKED_ITEM_NOT_FOUND' and 'LINKED_ITEM_NOT_STORED') so error handling is uniform.
1216-1232: ⚡ Quick winUnbounded query may degrade performance on busy campuses.
This query fetches all stored items for the campus without any limit. For campuses with thousands of stored items, this could cause high memory usage and slow response times.
Consider adding a reasonable limit or implementing batched processing:
♻️ Suggested improvement
const candidates = await prisma.item.findMany({ where: { campusId: claim.campusId, status: ItemStatus.stored, }, + take: 500, // Reasonable upper bound select: { itemId: true, category: true,Alternatively, pre-filter by category to reduce the candidate set:
const candidates = await prisma.item.findMany({ where: { campusId: claim.campusId, status: ItemStatus.stored, + category: { equals: claim.category, mode: 'insensitive' }, },🤖 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 `@backend/src/routes/claims.ts` around lines 1216 - 1232, The query that populates candidates via prisma.item.findMany (the variable candidates, using claim.campusId and ItemStatus.stored) is unbounded and can OOM on large campuses; change it to either (a) add a reasonable limit (use take/skip or limit/take with optional pagination) or (b) perform batched processing with a cursor loop to stream results, or (c) pre-filter by category/location using fields from the claim (e.g., category or locationFound) to reduce the result set. Update the call site that consumes candidates to handle pagination/batches or filtered results accordingly (look for usages of candidates in the surrounding function) so logic still works with partial/streamed sets.backend/src/routes/items.ts (4)
102-118: 💤 Low valueUnnecessary array spread in status filter (same as line 25).
Line 109 spreads
publicItemStatusesinto a new array. Since it's already an array constant, the spread is redundant.♻️ Simplify the filter
const rows = await prisma.item.groupBy({ by: ['category'], where: { - status: { in: [...publicItemStatuses] }, + status: { in: publicItemStatuses }, ...(campusId ? { campusId } : {}), },🤖 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 `@backend/src/routes/items.ts` around lines 102 - 118, Remove the redundant array spread when filtering by status in the prisma.item.groupBy call: instead of using status: { in: [...publicItemStatuses] } simply use status: { in: publicItemStatuses }; update the code around the groupBy invocation (the prisma.item.groupBy block) that builds the where clause (uses campusId and publicItemStatuses) to pass the array constant directly so no new array is created.
25-25: 💤 Low valueUnnecessary array spreads in status filters.
Lines 25 and 109 both spread
publicItemStatusesinto new arrays (in: [...publicItemStatuses]). SincepublicItemStatusesis already an array constant, the spread operator is redundant—Prisma'sinclause accepts the array directly.♻️ Simplify both filters
// Line 25 const where: Prisma.ItemWhereInput = { - status: { in: [...publicItemStatuses] }, + status: { in: publicItemStatuses }, }; // Line 109 const rows = await prisma.item.groupBy({ by: ['category'], where: { - status: { in: [...publicItemStatuses] }, + status: { in: publicItemStatuses }, ...(campusId ? { campusId } : {}), },🤖 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 `@backend/src/routes/items.ts` at line 25, The status filter uses an unnecessary array spread of the constant publicItemStatuses (e.g., status: { in: [...publicItemStatuses] }); update both occurrences to pass the array directly (status: { in: publicItemStatuses }) so Prisma receives the constant array without creating a new array; locate the two filters referencing publicItemStatuses in items route handlers and replace the spread form with the direct array reference.
61-79: ⚡ Quick winAdd pagination to prevent performance degradation with large datasets.
The
/public/itemsendpoint returns all matching stored items without limit or cursor-based pagination. As the number of stored items grows, this will cause slow response times, high memory usage, and poor user experience.Add pagination parameters (e.g.,
limit,offset, or cursor-based) to the query schema and Prisma query.💡 Example pagination approach
Update the validator to accept pagination params:
// In validators/items.ts export const publicItemsQuerySchema = z.object({ category: z.string().min(1).max(50).trim().optional(), campusId: z.uuid().optional(), limit: z.coerce.number().int().min(1).max(100).default(20), offset: z.coerce.number().int().min(0).default(0), });Then apply in the route:
const query = req.query as { category?: string; campusId?: string }; +const { limit, offset, ...filters } = query; const items = await prisma.item.findMany({ - where: buildPublicItemWhere(query), + where: buildPublicItemWhere(filters), select: publicItemSelect, orderBy: [{ dateFound: 'desc' }, { createdAt: 'desc' }], + take: limit, + skip: offset, });🤖 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 `@backend/src/routes/items.ts` around lines 61 - 79, The /public/items route returns all matches with prisma.item.findMany causing potential performance issues; update the publicItemsQuerySchema used by validateQuery to include pagination parameters (e.g., limit and offset with sane defaults and bounds, or a cursor-based param), then change the handler for the GET '/public/items' route to consume those params and pass them into prisma.item.findMany (use take and skip for limit/offset or cursor/skip for cursor pagination) while preserving where: buildPublicItemWhere(query), select: publicItemSelect, and the orderBy clauses; validate/coerce numeric inputs and enforce max limit to prevent abuse.
23-37: 💤 Low valueUnnecessary array spread in status filter.
Line 25 spreads
publicItemStatusesinto a new array. Since it's already an array constant, the spread is redundant. Prisma'sinclause accepts the array directly.♻️ Simplify the filter
function buildPublicItemWhere(query: { category?: string; campusId?: string }) { const where: Prisma.ItemWhereInput = { - status: { in: [...publicItemStatuses] }, + status: { in: publicItemStatuses }, };🤖 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 `@backend/src/routes/items.ts` around lines 23 - 37, The status filter in buildPublicItemWhere unnecessarily spreads publicItemStatuses into a new array; update the where.status.in assignment to pass publicItemStatuses directly (e.g., status: { in: publicItemStatuses }) so Prisma receives the array constant without an extra spread while keeping the same Prisma.ItemWhereInput shape.backend/src/routes/reportLinks.ts (3)
148-157: 💤 Low valueManual validation errors diverge from Zod's format across both endpoints.
Both the validate endpoint (lines 148-157) and submit endpoint (lines 240-249) manually construct token validation errors with a simplified
detailsstructure{ path, message }, whereas thevalidatemiddleware returns Zod's fullerror.issueswhich include fields likecode. For API consistency, extract this error construction into a shared helper that matches Zod's format, or document the intentional deviation.🤖 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 `@backend/src/routes/reportLinks.ts` around lines 148 - 157, The manual token error objects in the validate and submit endpoints diverge from Zod's error.issues shape; create a shared helper (e.g., buildZodIssue or formatValidationIssue) and replace the inline objects in the validate handler and submit handler with calls to that helper so they return a Zod-like issue object containing at least { code: 'custom', path: ['token'], message: 'Token is required' } (or an appropriate Zod code) and use that helper to populate the response details to match the middleware's error.issues format for consistency across validate and submit.
396-396: 💤 Low valueConsider relocating the export for better code organization.
The
publicItemStatusesconstant is exported at the bottom of the file after all route handlers. While this is functional, placing it near the top with other module-level constants (likereportLinkSelectandsubmitterSelectat lines 36-48) would improve readability and discoverability.🤖 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 `@backend/src/routes/reportLinks.ts` at line 396, Move the exported constant publicItemStatuses from the bottom of the file to the top module-level constants section near reportLinkSelect and submitterSelect so related configuration values are grouped together; locate the declaration publicItemStatuses = [ItemStatus.stored] as const and cut/paste it into the block where reportLinkSelect and submitterSelect are defined, keeping the export and type assertion unchanged and updating any nearby imports or references if necessary.
253-311: ⚖️ Poor tradeoffConsider parallel execution of pre-transaction queries for faster response.
Lines 253 and 283 execute sequential database queries to validate the report link and submitter. While the current approach provides clear, specific error messages, these queries are independent and could run in parallel using
Promise.allto reduce latency.♻️ Optional refactor for parallel queries
- const link = await prisma.reportLink.findUnique({ - where: { token }, - select: reportLinkSelect, - }); - - if (!link) { - res.status(404).json({ - code: 'REPORT_LINK_NOT_FOUND', - message: 'Report link does not exist.', - }); - return; - } - - if (link.isUsed) { - res.status(409).json({ - code: 'REPORT_LINK_USED', - message: 'Report link has already been used.', - }); - return; - } - - const now = new Date(); - if (link.expiresAt <= now) { - res.status(409).json({ - code: 'REPORT_LINK_EXPIRED', - message: 'Report link has expired.', - }); - return; - } - - const submitter = await prisma.user.findUnique({ - where: { userId: req.user!.user_id }, - select: submitterSelect, - }); + const [link, submitter] = await Promise.all([ + prisma.reportLink.findUnique({ + where: { token }, + select: reportLinkSelect, + }), + prisma.user.findUnique({ + where: { userId: req.user!.user_id }, + select: submitterSelect, + }), + ]); + + if (!link) { + res.status(404).json({ + code: 'REPORT_LINK_NOT_FOUND', + message: 'Report link does not exist.', + }); + return; + } + + if (link.isUsed) { + res.status(409).json({ + code: 'REPORT_LINK_USED', + message: 'Report link has already been used.', + }); + return; + } + + const now = new Date(); + if (link.expiresAt <= now) { + res.status(409).json({ + code: 'REPORT_LINK_EXPIRED', + message: 'Report link has expired.', + }); + return; + }Given the rate limiting (10 requests per 15 minutes), the performance gain is minimal, so this refactor is entirely optional.
🤖 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 `@backend/src/routes/reportLinks.ts` around lines 253 - 311, Parallelize the independent DB lookups by running prisma.reportLink.findUnique (using token and reportLinkSelect) and prisma.user.findUnique (using req.user!.user_id and submitterSelect) with Promise.all, then await both results and perform the same conditional checks (link missing, link.isUsed, link.expiresAt, submitter missing, submitter.isActive, campusId mismatch) in the same order to preserve existing specific responses; ensure you still use the exact identifiers reportLinkSelect, submitterSelect, token, and req.user!.user_id when invoking the queries and keep the same response codes/messages after the combined await.
🤖 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 `@backend/src/routes/claims.ts`:
- Around line 1395-1406: The tx.matchSuggestion.updateMany block currently
dismisses other confirmed matches unconditionally; move that block so it only
runs when the incoming status is a confirmation by wrapping or relocating it
inside the if (status === MatchStatus.confirmed) branch (use the existing status
variable, match.matchId/match.claimId context and MatchStatus.confirmed
constant), ensuring other flows (dismissed/rejected) do not trigger the
updateMany call.
In `@backend/src/routes/items.ts`:
- Line 8: Replace the local redefinition of the constant with an import of the
shared exported constant: remove the line declaring publicItemStatuses and add
an import for the exported publicItemStatuses from the module that currently
exports it (the reportLinks export), and update any references to continue using
publicItemStatuses; ensure the import uses the same name and const typing so
behavior is unchanged.
In `@backend/src/validators/reportLinks.ts`:
- Around line 6-10: getTodayAtMidnight() currently returns local-midnight which
mismatches the UTC-midnight value used for dateFound; change
getTodayAtMidnight() to produce a Date at 00:00:00 UTC (e.g., build with
Date.UTC(year, month, day) or call setUTCHours(0,0,0,0)) so both sides are in
UTC before the comparison with dateFound, and update any callers that assume
local-midnight to use the new UTC behavior (identify getTodayAtMidnight and the
dateFound comparison location to apply the fix).
---
Outside diff comments:
In `@backend/README.md`:
- Around line 106-127: Update the project structure in backend/README.md to
include the omitted validator and route files: add validators/reportLinks.ts and
validators/items.ts under the validators/ list and add routes/reportLinks.ts and
routes/items.ts under the routes/ list so the README matches existing code and
the API tables; ensure the short descriptions mirror the style used for other
entries (e.g., mention which endpoints are done/stubbed) and keep formatting
consistent with the surrounding tree.
---
Nitpick comments:
In `@backend/src/routes/claims.ts`:
- Around line 1017-1026: Make the error signaling consistent: when throwing
LINKED_ITEM_NOT_FOUND and LINKED_ITEM_NOT_STORED set both error.message and
error.name (e.g., new Error('LINKED_ITEM_NOT_FOUND') then error.name =
'LINKED_ITEM_NOT_FOUND' for the not-found case) so both throws use the same
pattern; then update the catch block that currently mixes checks of .name and
.message to check the same property (prefer checking .name for both
'LINKED_ITEM_NOT_FOUND' and 'LINKED_ITEM_NOT_STORED') so error handling is
uniform.
- Around line 1216-1232: The query that populates candidates via
prisma.item.findMany (the variable candidates, using claim.campusId and
ItemStatus.stored) is unbounded and can OOM on large campuses; change it to
either (a) add a reasonable limit (use take/skip or limit/take with optional
pagination) or (b) perform batched processing with a cursor loop to stream
results, or (c) pre-filter by category/location using fields from the claim
(e.g., category or locationFound) to reduce the result set. Update the call site
that consumes candidates to handle pagination/batches or filtered results
accordingly (look for usages of candidates in the surrounding function) so logic
still works with partial/streamed sets.
In `@backend/src/routes/items.ts`:
- Around line 102-118: Remove the redundant array spread when filtering by
status in the prisma.item.groupBy call: instead of using status: { in:
[...publicItemStatuses] } simply use status: { in: publicItemStatuses }; update
the code around the groupBy invocation (the prisma.item.groupBy block) that
builds the where clause (uses campusId and publicItemStatuses) to pass the array
constant directly so no new array is created.
- Line 25: The status filter uses an unnecessary array spread of the constant
publicItemStatuses (e.g., status: { in: [...publicItemStatuses] }); update both
occurrences to pass the array directly (status: { in: publicItemStatuses }) so
Prisma receives the constant array without creating a new array; locate the two
filters referencing publicItemStatuses in items route handlers and replace the
spread form with the direct array reference.
- Around line 61-79: The /public/items route returns all matches with
prisma.item.findMany causing potential performance issues; update the
publicItemsQuerySchema used by validateQuery to include pagination parameters
(e.g., limit and offset with sane defaults and bounds, or a cursor-based param),
then change the handler for the GET '/public/items' route to consume those
params and pass them into prisma.item.findMany (use take and skip for
limit/offset or cursor/skip for cursor pagination) while preserving where:
buildPublicItemWhere(query), select: publicItemSelect, and the orderBy clauses;
validate/coerce numeric inputs and enforce max limit to prevent abuse.
- Around line 23-37: The status filter in buildPublicItemWhere unnecessarily
spreads publicItemStatuses into a new array; update the where.status.in
assignment to pass publicItemStatuses directly (e.g., status: { in:
publicItemStatuses }) so Prisma receives the array constant without an extra
spread while keeping the same Prisma.ItemWhereInput shape.
In `@backend/src/routes/reportLinks.ts`:
- Around line 148-157: The manual token error objects in the validate and submit
endpoints diverge from Zod's error.issues shape; create a shared helper (e.g.,
buildZodIssue or formatValidationIssue) and replace the inline objects in the
validate handler and submit handler with calls to that helper so they return a
Zod-like issue object containing at least { code: 'custom', path: ['token'],
message: 'Token is required' } (or an appropriate Zod code) and use that helper
to populate the response details to match the middleware's error.issues format
for consistency across validate and submit.
- Line 396: Move the exported constant publicItemStatuses from the bottom of the
file to the top module-level constants section near reportLinkSelect and
submitterSelect so related configuration values are grouped together; locate the
declaration publicItemStatuses = [ItemStatus.stored] as const and cut/paste it
into the block where reportLinkSelect and submitterSelect are defined, keeping
the export and type assertion unchanged and updating any nearby imports or
references if necessary.
- Around line 253-311: Parallelize the independent DB lookups by running
prisma.reportLink.findUnique (using token and reportLinkSelect) and
prisma.user.findUnique (using req.user!.user_id and submitterSelect) with
Promise.all, then await both results and perform the same conditional checks
(link missing, link.isUsed, link.expiresAt, submitter missing,
submitter.isActive, campusId mismatch) in the same order to preserve existing
specific responses; ensure you still use the exact identifiers reportLinkSelect,
submitterSelect, token, and req.user!.user_id when invoking the queries and keep
the same response codes/messages after the combined await.
In `@backend/src/validators/reportLinks.ts`:
- Line 30: The additionalNotes schema uses .max(2000) before .trim(), so length
is checked on the untrimmed input; update the validator for additionalNotes (the
z.string() chain) to call .trim() before .max(2000) — e.g., change the chain to
z.string().trim().max(2000).optional() — so the trimmed length is what gets
validated.
🪄 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: 9fb2bffb-144c-4aa9-a4d2-6f3ba4f7a3a1
📒 Files selected for processing (8)
backend/README.mdbackend/src/index.tsbackend/src/routes/claims.tsbackend/src/routes/items.tsbackend/src/routes/reportLinks.tsbackend/src/validators/claims.tsbackend/src/validators/items.tsbackend/src/validators/reportLinks.ts
Comment on lines
+1395
to
+1406
| await tx.matchSuggestion.updateMany({ | ||
| where: { | ||
| claimId: match.claimId, | ||
| matchId: { not: match.matchId }, | ||
| status: MatchStatus.confirmed, | ||
| }, | ||
| data: { | ||
| status: MatchStatus.dismissed, | ||
| reviewedBy: actor.userId, | ||
| reviewedAt: new Date(), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Other confirmed matches are dismissed unconditionally, regardless of the review action.
This updateMany runs for all status updates (confirmed, dismissed, rejected), not just when confirming. If an admin dismisses or rejects a suggestion, any other previously confirmed matches would be incorrectly dismissed.
This block should be inside the if (status === MatchStatus.confirmed) branch:
🐛 Proposed fix
const updated = await prisma.$transaction(async (tx) => {
if (status === MatchStatus.confirmed) {
const item = await tx.item.findUnique({
where: { itemId: match.itemId },
select: { itemId: true, status: true },
});
if (!item) {
throw new Error('MATCH_ITEM_NOT_FOUND');
}
if (item.status !== ItemStatus.stored) {
const conflictError = new Error('MATCH_ITEM_NOT_STORED');
conflictError.name = 'MATCH_ITEM_NOT_STORED';
throw conflictError;
}
await tx.claim.update({
where: { claimId: match.claimId },
data: { itemId: match.itemId },
});
- }
- await tx.matchSuggestion.updateMany({
- where: {
- claimId: match.claimId,
- matchId: { not: match.matchId },
- status: MatchStatus.confirmed,
- },
- data: {
- status: MatchStatus.dismissed,
- reviewedBy: actor.userId,
- reviewedAt: new Date(),
- },
- });
+ await tx.matchSuggestion.updateMany({
+ where: {
+ claimId: match.claimId,
+ matchId: { not: match.matchId },
+ status: MatchStatus.confirmed,
+ },
+ data: {
+ status: MatchStatus.dismissed,
+ reviewedBy: actor.userId,
+ reviewedAt: new Date(),
+ },
+ });
+ }
return tx.matchSuggestion.update({🤖 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 `@backend/src/routes/claims.ts` around lines 1395 - 1406, The
tx.matchSuggestion.updateMany block currently dismisses other confirmed matches
unconditionally; move that block so it only runs when the incoming status is a
confirmation by wrapping or relocating it inside the if (status ===
MatchStatus.confirmed) branch (use the existing status variable,
match.matchId/match.claimId context and MatchStatus.confirmed constant),
ensuring other flows (dismissed/rejected) do not trigger the updateMany call.
| import { publicItemsQuerySchema } from '../validators/items'; | ||
|
|
||
| const router = Router(); | ||
| const publicItemStatuses = [ItemStatus.stored] as const; |
There was a problem hiding this comment.
Import publicItemStatuses instead of redefining it.
This constant is already exported from reportLinks.ts (line 396). Redefining it here creates a maintainability risk—if the allowed public statuses change, both locations must be updated.
♻️ Proposed fix to import the shared constant
import { Router } from 'express';
import { ItemStatus, Prisma } from '`@prisma/client`';
import { prisma } from '../db';
import { validateQuery } from '../validators/shared';
import { publicItemsQuerySchema } from '../validators/items';
+import { publicItemStatuses } from './reportLinks';
const router = Router();
-const publicItemStatuses = [ItemStatus.stored] as const;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const publicItemStatuses = [ItemStatus.stored] as const; | |
| import { Router } from 'express'; | |
| import { ItemStatus, Prisma } from '`@prisma/client`'; | |
| import { prisma } from '../db'; | |
| import { validateQuery } from '../validators/shared'; | |
| import { publicItemsQuerySchema } from '../validators/items'; | |
| import { publicItemStatuses } from './reportLinks'; | |
| const router = Router(); |
🤖 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 `@backend/src/routes/items.ts` at line 8, Replace the local redefinition of the
constant with an import of the shared exported constant: remove the line
declaring publicItemStatuses and add an import for the exported
publicItemStatuses from the module that currently exports it (the reportLinks
export), and update any references to continue using publicItemStatuses; ensure
the import uses the same name and const typing so behavior is unchanged.
Comment on lines
+6
to
+10
| function getTodayAtMidnight() { | ||
| const today = new Date(); | ||
| today.setHours(0, 0, 0, 0); | ||
| return today; | ||
| } |
There was a problem hiding this comment.
Timezone inconsistency in date comparison.
getTodayAtMidnight() returns a Date at midnight in the server's local timezone, but dateFound (line 22) is transformed to UTC midnight. When these are compared on line 23, the result depends on the server's timezone.
Example failure:
- Server timezone: PST (UTC-8), today is June 11
- User submits
dateFound: "2026-06-12" - Transformed:
2026-06-12T00:00:00.000Z(midnight UTC) getTodayAtMidnight():2026-06-11T00:00:00-08:00→2026-06-11T08:00:00.000Z(8am UTC)- Comparison:
2026-06-12T00:00:00.000Z <= 2026-06-11T08:00:00.000Z→ false (incorrectly rejects tomorrow's date)
🔧 Proposed fix: use UTC for both dates
function getTodayAtMidnight() {
const today = new Date();
- today.setHours(0, 0, 0, 0);
+ today.setUTCHours(0, 0, 0, 0);
return today;
}Or alternatively, compare date strings before transformation:
dateFound: z.iso
.date()
+ .refine((value) => value <= new Date().toISOString().split('T')[0], {
+ message: 'dateFound cannot be in the future',
+ })
.transform((value) => new Date(`${value}T00:00:00.000Z`))
- .refine((value) => value <= getTodayAtMidnight(), {
- message: 'dateFound cannot be in the future',
- }),Also applies to: 23-23
🤖 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 `@backend/src/validators/reportLinks.ts` around lines 6 - 10,
getTodayAtMidnight() currently returns local-midnight which mismatches the
UTC-midnight value used for dateFound; change getTodayAtMidnight() to produce a
Date at 00:00:00 UTC (e.g., build with Date.UTC(year, month, day) or call
setUTCHours(0,0,0,0)) so both sides are in UTC before the comparison with
dateFound, and update any callers that assume local-midnight to use the new UTC
behavior (identify getTodayAtMidnight and the dateFound comparison location to
apply the fix).
Owner
|
@coderabbitai review the pr again |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/README.md (1)
96-130:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate Project Structure documentation to include reportLinks and items validators/routes.
The Project Structure section lists only
claims.tsvalidator and route, but the API Endpoints table (below) documents done routes for report-links and items. The structure documentation is incomplete and doesn't match the implemented code surface area.The following entries are missing from the
validators/androutes/sections:
src/validators/reportLinks.ts(for/api/report-links/*endpoints)src/validators/items.ts(for/api/public/itemsand/api/items/category-statsendpoints)src/routes/reportLinks.tssrc/routes/items.ts📝 Proposed additions to Project Structure
├── validators/ │ ├── shared.ts # validate() and validateQuery() middleware helpers │ ├── auth.ts # Zod schemas: loginSchema, registerSchema, refreshSchema, logoutSchema │ ├── claims.ts # Zod schemas for claim routes +│ ├── reportLinks.ts # Zod schemas for report-link routes +│ ├── items.ts # Zod schemas for items query validation │ └── users.ts # Zod schemas: replaceProfileSchema, updateProfileSchema, createUserSchema, listUsersQuerySchemaAnd in the routes section:
├── routes/ │ ├── health.ts # GET /api/health │ ├── auth.ts # POST /api/auth/login|register (done) · refresh|logout (stub) │ ├── claims.ts # Claim lifecycle routes +│ ├── reportLinks.ts # Report-link validation and found-item submission +│ ├── items.ts # Public item listing and category statistics │ ├── users.ts # GET|PUT /api/users/me (done) · PATCH stubs │ └── admin/ │ └── users.ts # Admin user management stubs🤖 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 `@backend/README.md` around lines 96 - 130, The Project Structure documentation in the README is incomplete and does not reflect all implemented validators and routes. Add the missing reportLinks.ts and items.ts entries to both the validators/ and routes/ sections to match the actual codebase structure. In the validators/ section after the claims.ts entry, add the reportLinks and items validators. In the routes/ section after the claims.ts entry, add the corresponding reportLinks and items route files. Ensure the formatting and indentation remain consistent with the existing entries in these sections.
🤖 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 `@backend/src/routes/claims.ts`:
- Around line 967-1057: The code validates claim.status and claim.itemId before
the transaction begins, but the transactional write in the prisma.$transaction
block only matches claimId in the WHERE clause. This creates a race condition
where two concurrent reviewers can both pass validation against stale data and
then apply conflicting updates. Fix this by making the tx.claim.update call
include the original validated status and itemId in its WHERE clause (using
compare-and-swap semantics), so the update fails if another reviewer has already
modified the claim's status or itemId since validation. Alternatively, re-read
the claim at the start of the transaction to ensure you're validating and
updating against current data.
- Around line 848-886: Item attachment to claims is duplicated across two code
paths and neither validates that an item is not already attached to another
active claim, allowing multiple claims to point at the same stored item. Create
a transactional helper function that centralizes the item attachment logic and
enforces all invariants including a check that the itemId is not already
attached to another active claim. At the anchor location
(backend/src/routes/claims.ts#L848-L886), replace the validation checks for
item.status, item.campusId, and the prisma.claim.update call with a call to this
new helper. At the sibling location (backend/src/routes/claims.ts#L1373-L1392),
call this same helper before confirming the suggestion so that both paths
enforce consistent rules.
- Around line 1034-1048: The tx.claim.update() call unconditionally overwrites
`reviewedBy` and `reviewedAt` fields even when the claim status is being set to
picked_up, which erases the original reviewer information. Instead of always
including `reviewedBy` and `reviewedAt` at the top level of the data object,
conditionally include them only when the status is not ClaimStatus.picked_up,
similar to how the pickedUpAt and verifiedBy fields are already conditionally
included. This ensures the original review information is preserved and only the
new pickup verification (verifiedBy) is recorded when the item is picked up.
In `@backend/src/routes/items.ts`:
- Around line 68-72: The findMany query in the items.ts file lacks a take
parameter to limit result set size, which allows unbounded reads on the public
endpoint. Add a take property with a reasonable maximum limit (such as 100) to
the findMany call that includes the where, select, and orderBy clauses to
prevent performance degradation as item volume grows.
In `@backend/src/validators/claims.ts`:
- Around line 33-55: The string validators have `.trim()` chained after
`.min()`, allowing whitespace-only inputs to pass validation before being
trimmed to empty. Reorder the validation chain to call `.trim()` before `.min()`
in all affected fields. In backend/src/validators/claims.ts (lines 33-55), move
`.trim()` before `.min(1)` for the fields: category, description, locationLost,
and rejectionReason. In backend/src/validators/reportLinks.ts (lines 3-19), move
`.trim()` before `.min(1)` for the fields: categorySchema, locationSchema,
token, and itemDescription. In backend/src/validators/items.ts (lines 4-4), move
`.trim()` before `.min(1)` for the field: category. This ensures length
validation occurs on normalized input, preventing whitespace-only values from
passing validation.
In `@backend/src/validators/items.ts`:
- Line 4: In the category field validator definition, reorder the schema chain
so that trim() is called before the min() and max() length validations.
Currently, the chain is z.string().min(1).max(50).trim().optional(), which
allows whitespace-only strings to pass the minimum length check before being
trimmed. Move the trim() call to immediately after z.string() so that length
validation happens on the already-trimmed string, preventing whitespace-only
strings from passing validation.
---
Outside diff comments:
In `@backend/README.md`:
- Around line 96-130: The Project Structure documentation in the README is
incomplete and does not reflect all implemented validators and routes. Add the
missing reportLinks.ts and items.ts entries to both the validators/ and routes/
sections to match the actual codebase structure. In the validators/ section
after the claims.ts entry, add the reportLinks and items validators. In the
routes/ section after the claims.ts entry, add the corresponding reportLinks and
items route files. Ensure the formatting and indentation remain consistent with
the existing entries in these sections.
🪄 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: 4de04857-9c85-4516-b6f9-d5d8edfb4a98
📒 Files selected for processing (8)
backend/README.mdbackend/src/index.tsbackend/src/routes/claims.tsbackend/src/routes/items.tsbackend/src/routes/reportLinks.tsbackend/src/validators/claims.tsbackend/src/validators/items.tsbackend/src/validators/reportLinks.ts
Comment on lines
+848
to
+886
| const { itemId } = req.body as { itemId: string }; | ||
| const item = await prisma.item.findUnique({ | ||
| where: { itemId }, | ||
| select: { | ||
| itemId: true, | ||
| campusId: true, | ||
| status: true, | ||
| }, | ||
| }); | ||
|
|
||
| if (!item) { | ||
| res.status(404).json({ | ||
| code: 'ITEM_NOT_FOUND', | ||
| message: 'Item not found.', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (item.status !== ItemStatus.stored) { | ||
| res.status(409).json({ | ||
| code: 'ITEM_NOT_STORED', | ||
| message: 'Only stored items can be linked to a claim.', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (item.campusId !== claim.campusId) { | ||
| res.status(409).json({ | ||
| code: 'ITEM_CAMPUS_MISMATCH', | ||
| message: 'The item campus must match the claim campus.', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const updated = await prisma.claim.update({ | ||
| where: { claimId: claim.claimId }, | ||
| data: { itemId: item.itemId }, | ||
| select: claimDetailSelect, | ||
| }); |
There was a problem hiding this comment.
Centralize the claim-item attachment rules before writing claim.itemId.
These two handlers attach an item to a claim but enforce different subsets of the invariants, and neither prevents another active claim from already holding the same itemId. Today a confirmed suggestion can bypass the picked_up relink guard, and either path can leave multiple claims pointing at the same stored item until a later pickup fails.
backend/src/routes/claims.ts#L848-L886: move the attachment into a transactional helper that also rejects items already attached to another active claim.backend/src/routes/claims.ts#L1373-L1392: reuse that same helper before confirming a suggestion so confirm cannot relink apicked_upclaim or double-assign the item.
📍 Affects 1 file
backend/src/routes/claims.ts#L848-L886(this comment)backend/src/routes/claims.ts#L1373-L1392
🤖 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 `@backend/src/routes/claims.ts` around lines 848 - 886, Item attachment to
claims is duplicated across two code paths and neither validates that an item is
not already attached to another active claim, allowing multiple claims to point
at the same stored item. Create a transactional helper function that centralizes
the item attachment logic and enforces all invariants including a check that the
itemId is not already attached to another active claim. At the anchor location
(backend/src/routes/claims.ts#L848-L886), replace the validation checks for
item.status, item.campusId, and the prisma.claim.update call with a call to this
new helper. At the sibling location (backend/src/routes/claims.ts#L1373-L1392),
call this same helper before confirming the suggestion so that both paths
enforce consistent rules.
Comment on lines
+967
to
+1057
| const claim = await prisma.claim.findUnique({ | ||
| where: { claimId: params.data.claimId }, | ||
| select: claimDetailSelect, | ||
| }); | ||
| if (!claim) { | ||
| res.status(404).json({ | ||
| code: 'CLAIM_NOT_FOUND', | ||
| message: 'Claim not found.', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const { status, rejectionReason } = req.body as { | ||
| status: ClaimStatus; | ||
| rejectionReason?: string; | ||
| }; | ||
|
|
||
| if (!validStatusTransitions[claim.status].includes(status)) { | ||
| res.status(409).json({ | ||
| code: 'INVALID_STATUS_TRANSITION', | ||
| message: `Cannot change claim status from ${claim.status} to ${status}.`, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (status === ClaimStatus.rejected && !rejectionReason) { | ||
| res.status(400).json({ | ||
| code: 'REJECTION_REASON_REQUIRED', | ||
| message: 'A rejection reason is required when rejecting a claim.', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| (status === ClaimStatus.approved || status === ClaimStatus.picked_up) && | ||
| !claim.itemId | ||
| ) { | ||
| res.status(409).json({ | ||
| code: 'CLAIM_ITEM_REQUIRED', | ||
| message: 'A claim must be linked to an item before it can advance.', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const updated = await prisma.$transaction(async (tx) => { | ||
| if (status === ClaimStatus.picked_up && claim.itemId) { | ||
| const item = await tx.item.findUnique({ | ||
| where: { itemId: claim.itemId }, | ||
| select: { itemId: true, status: true }, | ||
| }); | ||
|
|
||
| if (!item) { | ||
| throw new Error('LINKED_ITEM_NOT_FOUND'); | ||
| } | ||
|
|
||
| if (item.status !== ItemStatus.stored) { | ||
| const conflictError = new Error('LINKED_ITEM_NOT_STORED'); | ||
| conflictError.name = 'LINKED_ITEM_NOT_STORED'; | ||
| throw conflictError; | ||
| } | ||
|
|
||
| await tx.item.update({ | ||
| where: { itemId: item.itemId }, | ||
| data: { status: ItemStatus.claimed }, | ||
| }); | ||
| } | ||
|
|
||
| const nextClaim = await tx.claim.update({ | ||
| where: { claimId: claim.claimId }, | ||
| data: { | ||
| status, | ||
| rejectionReason: | ||
| status === ClaimStatus.rejected ? rejectionReason : null, | ||
| reviewedBy: actor.userId, | ||
| reviewedAt: new Date(), | ||
| ...(status === ClaimStatus.picked_up | ||
| ? { | ||
| pickedUpAt: new Date(), | ||
| verifiedBy: actor.userId, | ||
| } | ||
| : {}), | ||
| }, | ||
| select: claimDetailSelect, | ||
| }); | ||
|
|
||
| await tx.notification.create({ | ||
| data: createClaimStatusNotificationInput(nextClaim, status), | ||
| }); | ||
|
|
||
| return nextClaim; | ||
| }); |
There was a problem hiding this comment.
Make the status write conditional on the state you validated.
claim.status and claim.itemId are checked before the transaction, but the transactional write only matches claimId. Two reviewers can race on the same claim, both pass validation from the same stale row, and then apply conflicting transitions with mismatched item updates/notifications. Re-read the claim inside the transaction or use a compare-and-swap update that fails when the current status/item link has changed.
🤖 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 `@backend/src/routes/claims.ts` around lines 967 - 1057, The code validates
claim.status and claim.itemId before the transaction begins, but the
transactional write in the prisma.$transaction block only matches claimId in the
WHERE clause. This creates a race condition where two concurrent reviewers can
both pass validation against stale data and then apply conflicting updates. Fix
this by making the tx.claim.update call include the original validated status
and itemId in its WHERE clause (using compare-and-swap semantics), so the update
fails if another reviewer has already modified the claim's status or itemId
since validation. Alternatively, re-read the claim at the start of the
transaction to ensure you're validating and updating against current data.
Comment on lines
+1034
to
+1048
| const nextClaim = await tx.claim.update({ | ||
| where: { claimId: claim.claimId }, | ||
| data: { | ||
| status, | ||
| rejectionReason: | ||
| status === ClaimStatus.rejected ? rejectionReason : null, | ||
| reviewedBy: actor.userId, | ||
| reviewedAt: new Date(), | ||
| ...(status === ClaimStatus.picked_up | ||
| ? { | ||
| pickedUpAt: new Date(), | ||
| verifiedBy: actor.userId, | ||
| } | ||
| : {}), | ||
| }, |
There was a problem hiding this comment.
Don’t overwrite reviewedBy when recording pickup verification.
verifiedBy already captures who handed the item over. Updating reviewedBy/reviewedAt again on picked_up erases the original reviewer from the claim record.
Suggested fix
const nextClaim = await tx.claim.update({
where: { claimId: claim.claimId },
data: {
status,
rejectionReason:
status === ClaimStatus.rejected ? rejectionReason : null,
- reviewedBy: actor.userId,
- reviewedAt: new Date(),
+ ...(status !== ClaimStatus.picked_up
+ ? {
+ reviewedBy: actor.userId,
+ reviewedAt: new Date(),
+ }
+ : {}),
...(status === ClaimStatus.picked_up
? {
pickedUpAt: new Date(),
verifiedBy: actor.userId,
}
: {}),
},
select: claimDetailSelect,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const nextClaim = await tx.claim.update({ | |
| where: { claimId: claim.claimId }, | |
| data: { | |
| status, | |
| rejectionReason: | |
| status === ClaimStatus.rejected ? rejectionReason : null, | |
| reviewedBy: actor.userId, | |
| reviewedAt: new Date(), | |
| ...(status === ClaimStatus.picked_up | |
| ? { | |
| pickedUpAt: new Date(), | |
| verifiedBy: actor.userId, | |
| } | |
| : {}), | |
| }, | |
| const nextClaim = await tx.claim.update({ | |
| where: { claimId: claim.claimId }, | |
| data: { | |
| status, | |
| rejectionReason: | |
| status === ClaimStatus.rejected ? rejectionReason : null, | |
| ...(status !== ClaimStatus.picked_up | |
| ? { | |
| reviewedBy: actor.userId, | |
| reviewedAt: new Date(), | |
| } | |
| : {}), | |
| ...(status === ClaimStatus.picked_up | |
| ? { | |
| pickedUpAt: new Date(), | |
| verifiedBy: actor.userId, | |
| } | |
| : {}), | |
| }, |
🤖 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 `@backend/src/routes/claims.ts` around lines 1034 - 1048, The tx.claim.update()
call unconditionally overwrites `reviewedBy` and `reviewedAt` fields even when
the claim status is being set to picked_up, which erases the original reviewer
information. Instead of always including `reviewedBy` and `reviewedAt` at the
top level of the data object, conditionally include them only when the status is
not ClaimStatus.picked_up, similar to how the pickedUpAt and verifiedBy fields
are already conditionally included. This ensures the original review information
is preserved and only the new pickup verification (verifiedBy) is recorded when
the item is picked up.
Comment on lines
+68
to
+72
| const items = await prisma.item.findMany({ | ||
| where: buildPublicItemWhere(query), | ||
| select: publicItemSelect, | ||
| orderBy: [{ dateFound: 'desc' }, { createdAt: 'desc' }], | ||
| }); |
There was a problem hiding this comment.
Bound the public list query to prevent unbounded reads.
Line 68 calls findMany on a public endpoint without a take cap, which can degrade DB and API performance as item volume grows.
♻️ Minimal safeguard
const router = Router();
const publicItemStatuses = [ItemStatus.stored] as const;
+const PUBLIC_ITEMS_MAX_RESULTS = 100;
@@
const items = await prisma.item.findMany({
where: buildPublicItemWhere(query),
select: publicItemSelect,
orderBy: [{ dateFound: 'desc' }, { createdAt: 'desc' }],
+ take: PUBLIC_ITEMS_MAX_RESULTS,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const items = await prisma.item.findMany({ | |
| where: buildPublicItemWhere(query), | |
| select: publicItemSelect, | |
| orderBy: [{ dateFound: 'desc' }, { createdAt: 'desc' }], | |
| }); | |
| const router = Router(); | |
| const publicItemStatuses = [ItemStatus.stored] as const; | |
| const PUBLIC_ITEMS_MAX_RESULTS = 100; |
Suggested change
| const items = await prisma.item.findMany({ | |
| where: buildPublicItemWhere(query), | |
| select: publicItemSelect, | |
| orderBy: [{ dateFound: 'desc' }, { createdAt: 'desc' }], | |
| }); | |
| const items = await prisma.item.findMany({ | |
| where: buildPublicItemWhere(query), | |
| select: publicItemSelect, | |
| orderBy: [{ dateFound: 'desc' }, { createdAt: 'desc' }], | |
| take: PUBLIC_ITEMS_MAX_RESULTS, | |
| }); |
🤖 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 `@backend/src/routes/items.ts` around lines 68 - 72, The findMany query in the
items.ts file lacks a take parameter to limit result set size, which allows
unbounded reads on the public endpoint. Add a take property with a reasonable
maximum limit (such as 100) to the findMany call that includes the where,
select, and orderBy clauses to prevent performance degradation as item volume
grows.
Comment on lines
+33
to
+55
| category: z.string().min(1).max(50).trim(), | ||
| description: z.string().min(1).max(2000).trim(), | ||
| dateLost: optionalDateSchema, | ||
| locationLost: z.string().min(1).max(255).trim().optional(), | ||
| }); | ||
|
|
||
| export const listClaimsQuerySchema = z.object({ | ||
| status: z.enum(claimStatusValues).optional(), | ||
| campusId: z.uuid().optional(), | ||
| studentId: z.uuid().optional(), | ||
| itemId: z.uuid().optional(), | ||
| cursor: z.uuid().optional(), | ||
| limit: z.coerce.number().int().min(1).max(50).default(20), | ||
| }); | ||
|
|
||
| export const linkClaimItemSchema = z.object({ | ||
| itemId: z.uuid(), | ||
| }); | ||
|
|
||
| export const updateClaimStatusSchema = z.object({ | ||
| status: z.enum(claimStatusValues), | ||
| rejectionReason: z.string().min(1).max(2000).trim().optional(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the claims validator file around lines 33-55
head -60 backend/src/validators/claims.ts | tail -30Repository: 86unj/Foundit
Length of output: 923
🏁 Script executed:
# Read the reportLinks validator file around lines 3-19
head -25 backend/src/validators/reportLinks.tsRepository: 86unj/Foundit
Length of output: 790
🏁 Script executed:
# Read the items validator file to check line 4
head -10 backend/src/validators/items.tsRepository: 86unj/Foundit
Length of output: 226
Move .trim() before .min() in all string validators: whitespace-only values pass length validation before normalization.
Across three validator files, required and optional string fields have .trim() chained after .min(1), allowing whitespace-only inputs to pass validation. A string of spaces passes .min(1) before being trimmed to empty, violating the semantic constraint of non-empty normalized data.
backend/src/validators/claims.ts:category,description,locationLost,rejectionReasonbackend/src/validators/reportLinks.ts:categorySchema,locationSchema,token,itemDescriptionbackend/src/validators/items.ts:category
Reorder as .trim().min(1).max(...) to validate length on normalized input.
📍 Affects 3 files
backend/src/validators/claims.ts#L33-L55(this comment)backend/src/validators/reportLinks.ts#L3-L19backend/src/validators/items.ts#L4-L4
🤖 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 `@backend/src/validators/claims.ts` around lines 33 - 55, The string validators
have `.trim()` chained after `.min()`, allowing whitespace-only inputs to pass
validation before being trimmed to empty. Reorder the validation chain to call
`.trim()` before `.min()` in all affected fields. In
backend/src/validators/claims.ts (lines 33-55), move `.trim()` before `.min(1)`
for the fields: category, description, locationLost, and rejectionReason. In
backend/src/validators/reportLinks.ts (lines 3-19), move `.trim()` before
`.min(1)` for the fields: categorySchema, locationSchema, token, and
itemDescription. In backend/src/validators/items.ts (lines 4-4), move `.trim()`
before `.min(1)` for the field: category. This ensures length validation occurs
on normalized input, preventing whitespace-only values from passing validation.
| import { z } from 'zod'; | ||
|
|
||
| export const publicItemsQuerySchema = z.object({ | ||
| category: z.string().min(1).max(50).trim().optional(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists and read the relevant content
if [ -f "backend/src/validators/items.ts" ]; then
echo "=== File content ==="
cat -n "backend/src/validators/items.ts"
else
echo "File not found at backend/src/validators/items.ts"
# Try to find it
find . -name "items.ts" -type f 2>/dev/null | head -20
fiRepository: 86unj/Foundit
Length of output: 289
🏁 Script executed:
#!/bin/bash
# First, search for where this schema is used
echo "=== Searching for publicItemsQuerySchema usage ==="
rg "publicItemsQuerySchema" --type ts --type tsx -B 2 -A 2
# Also search for any imports from items.ts
echo ""
echo "=== Searching for imports from validators/items ==="
rg "from.*validators/items" --type ts --type tsx -B 1 -A 3Repository: 86unj/Foundit
Length of output: 235
🏁 Script executed:
#!/bin/bash
# Search for usage with correct file types
echo "=== Searching for publicItemsQuerySchema usage ==="
rg "publicItemsQuerySchema" --type ts -B 2 -A 2
echo ""
echo "=== Searching for imports from validators/items ==="
rg "from.*validators/items|from.*items" --type ts -B 1 -A 3
# Also check all TypeScript files in src
echo ""
echo "=== Checking what other .ts files import from validators ==="
rg "from.*validators" --type ts | head -20Repository: 86unj/Foundit
Length of output: 3486
🏁 Script executed:
#!/bin/bash
# Check the validateQuery function to understand how the validation is used
echo "=== Checking validateQuery implementation ==="
cat -n "backend/src/validators/shared.ts"
# Also check how the category is used in the route handler
echo ""
echo "=== Checking how category is used in items route ==="
sed -n '1,80p' "backend/src/routes/items.ts" | cat -nRepository: 86unj/Foundit
Length of output: 3895
🏁 Script executed:
#!/bin/bash
# Test Zod validation behavior to verify the chain order concern
python3 - <<'PY'
import subprocess
import json
import os
# Create a test file to verify Zod behavior
test_code = """
const { z } = require('zod');
// Test case 1: min().trim() order
const schema1 = z.string().min(1).max(50).trim().optional();
// Test case 2: trim().min() order (suggested fix)
const schema2 = z.string().trim().min(1).max(50).optional();
// Test inputs
const testCases = [
" ", // whitespace only
" valid ", // whitespace with valid content
"", // empty string
"test", // normal string
];
console.log("=== Test Case 1: .min(1).max(50).trim().optional() ===");
testCases.forEach(input => {
const result = schema1.safeParse(input);
console.log(`Input: "${input}" (length: ${input.length}) => Success: ${result.success}, Data: "${result.data}"`);
});
console.log("\\n=== Test Case 2: .trim().min(1).max(50).optional() ===");
testCases.forEach(input => {
const result = schema2.safeParse(input);
console.log(`Input: "${input}" (length: ${input.length}) => Success: ${result.success}, Data: "${result.data}"`);
});
"""
# Write test file
with open('/tmp/test_zod.js', 'w') as f:
f.write(test_code)
# Run the test with Node.js
result = subprocess.run(['node', '/tmp/test_zod.js'], capture_output=True, text=True, cwd='/root/Foundit')
print(result.stdout)
if result.stderr:
print("STDERR:", result.stderr)
PYRepository: 86unj/Foundit
Length of output: 620
🏁 Script executed:
#!/bin/bash
# Test Zod behavior directly with node
node << 'JS'
const { z } = require('zod');
// Test case 1: min().trim() order (current code)
const schema1 = z.string().min(1).max(50).trim().optional();
// Test case 2: trim().min() order (suggested fix)
const schema2 = z.string().trim().min(1).max(50).optional();
// Test inputs
const testCases = [
" ", // whitespace only (3 chars)
" valid ", // whitespace with valid content
"", // empty string
"test", // normal string
];
console.log("=== Test Case 1: .min(1).max(50).trim().optional() ===");
testCases.forEach(input => {
const result = schema1.safeParse(input);
console.log(`Input: "${input}" (len: ${input.length}) => Success: ${result.success}, Data: "${result.data}"`);
});
console.log("\n=== Test Case 2: .trim().min(1).max(50).optional() ===");
testCases.forEach(input => {
const result = schema2.safeParse(input);
console.log(`Input: "${input}" (len: ${input.length}) => Success: ${result.success}, Data: "${result.data}"`);
});
// Also test with undefined to show optional behavior
console.log("\n=== undefined (optional field not provided) ===");
console.log("Schema 1:", schema1.safeParse(undefined));
console.log("Schema 2:", schema2.safeParse(undefined));
JSRepository: 86unj/Foundit
Length of output: 909
🌐 Web query:
Zod validation chain order .min().trim() vs .trim().min()
💡 Result:
In Zod, the order of chained methods is significant because validation and transformation steps are executed in the order they are defined [1][2]. When working with string validation, you should almost always place.trim before length constraints like.min,.max, or.length [1]..trim.min(1) (Recommended) This approach trims the input string first, then checks the length of the resulting value [1]. - If a user submits a string consisting only of whitespace (e.g., " "), the string is trimmed to "" (empty string). - The subsequent.min(1) check then correctly fails because the trimmed string has a length of 0 [1]..min(1).trim (Usually Unintended) This approach checks the length of the original, untrimmed string first, then trims it [1]. - If a user submits a string of spaces (e.g., " "), the original string has length 1 or more (depending on how many spaces), so it passes the.min check [1]. - The string is then trimmed afterward, which is typically not the desired behavior for form validation where you want to treat whitespace-only inputs as empty [1][3]. Summary of Behavior: - Order Matters: Zod processes methods sequentially [1][2]. - Best Practice: Use.trim as early as possible in your string schema to ensure all subsequent validation rules operate on the cleaned, intended data [1]. - Alternative/Advanced Control: For more complex scenarios, you can use.pipe to enforce a strict pipeline of transformations and validations, ensuring one operation fully completes before the next begins [4][5][6].
Citations:
- 1: https://github.com/marcalexiei/eslint-zod/blob/HEAD/plugins/eslint-plugin-zod/docs/rules/prefer-trim-before-string-length-checks.md
- 2: https://readoss.com/en/colinhacks/zod/inside-validation-pipeline-parsing-checks-error-collection
- 3: how to trim string before do validation? colinhacks/zod#614
- 4: Reconsider deprecation of .email() (or similar) methods - Loss of validation order control colinhacks/zod#5043
- 5: Zod transformations understanding colinhacks/zod#5734
- 6: Error message order or priority colinhacks/zod#3960
Reorder .trim() before length validation to prevent whitespace-only strings from passing validation.
In line 4, .min(1).max(50).trim().optional() allows whitespace-only strings to pass the minimum length check before trimming. For example, a string of only spaces (e.g., " ") passes .min(1) with its original length, then gets trimmed to "" (empty string), which then flows through the query logic as an empty filter value. This should be prevented by trimming first.
Suggested fix
- category: z.string().min(1).max(50).trim().optional(),
+ category: z.string().trim().min(1).max(50).optional(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| category: z.string().min(1).max(50).trim().optional(), | |
| category: z.string().trim().min(1).max(50).optional(), |
🤖 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 `@backend/src/validators/items.ts` at line 4, In the category field validator
definition, reorder the schema chain so that trim() is called before the min()
and max() length validations. Currently, the chain is
z.string().min(1).max(50).trim().optional(), which allows whitespace-only
strings to pass the minimum length check before being trimmed. Move the trim()
call to immediately after z.string() so that length validation happens on the
already-trimmed string, preventing whitespace-only strings from passing
validation.
This was referenced Jul 2, 2026
Merged
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
2 participants
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.
Summary by CodeRabbit